On Fri, 31 Aug 2018 07:57:24 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Cornelia Huck <coh...@redhat.com> 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 <coh...@redhat.com>
> > ---
> > 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 <arm...@redhat.com>
Thanks!
