Re: [qemu-s390x] [Qemu-devel] [PATCH v2 1/2] qemu-error: add {error, warn}_report_once_cond

[Cornelia Huck]

On Fri, 31 Aug 2018 07:57:24 +0200
Markus Armbruster <address@hidden> wrote:

> Cornelia Huck <address@hidden> writes:
> 
> > Add two functions to print an error/warning report once depending
> > on a passed-in condition variable and flip it if printed. This is
> > useful if you want to print a message not once-globally, but e.g.
> > once-per-device.
> >
> > Inspired by warn_once() in hw/vfio/ccw.c, which has been replaced
> > with warn_report_once_cond().
> >
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> >  hw/vfio/ccw.c               | 18 +++--------------
> >  include/qemu/error-report.h |  5 +++++
> >  util/qemu-error.c           | 48 
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 56 insertions(+), 15 deletions(-)

> > +/*
> > + * If *printed is false, print an error message to current monitor if we
> > + * have one, else to stderr, and flip *printed to true.  
> 
> I like function contracts to state the function's purpose in one line if
> at all possible.  Perhaps:
> 
>     * Like error_report(), except print just once.
>     * If *printed is false, print the message, and flip *printed to true.
> 
> > + * Returns false if message was not printed, else true.  
> 
> Uh, confusing negative.  What about
> 
>     * Return whether the message was printed.
> 
> Happy to make these tweaks in my tree.

Sure, these sound good to me.

> 
> > + * Format arguments like sprintf().  The resulting message should be
> > + * a single phrase, with no newline or trailing punctuation.
> > + * Prepend the current location and append a newline.
> > + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> > + */
> > +bool error_report_once_cond(bool *printed, const char *fmt, ...)
> > +{
> > +    va_list ap;
> > +
> > +    assert(printed);
> > +    if (*printed) {
> > +        return false;
> > +    }
> > +    *printed = true;
> > +    va_start(ap, fmt);
> > +    vreport(REPORT_TYPE_ERROR, fmt, ap);
> > +    va_end(ap);
> > +    return true;
> > +}
> > +
> > +/*
> > + * If *printed is false, print a warning message to current monitor if we
> > + * have one, else to stderr, and flip *printed to true.
> > + * Returns false if message was not printed, else true.
> > + * Format arguments like sprintf().  The resulting message should be
> > + * a single phrase, with no newline or trailing punctuation.
> > + * Prepend the current location and append a newline.
> > + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> > + */
> > +bool warn_report_once_cond(bool *printed, const char *fmt, ...)
> > +{
> > +    va_list ap;
> > +
> > +    assert(printed);
> > +    if (*printed) {
> > +        return false;
> > +    }
> > +    *printed = true;
> > +    va_start(ap, fmt);
> > +    vreport(REPORT_TYPE_WARNING, fmt, ap);
> > +    va_end(ap);
> > +    return true;
> > +}  
> 
> Preferably with improved comments:
> Reviewed-by: Markus Armbruster <address@hidden>

Thanks!