Re: [Qemu-devel] [RFC v3 2/3] qemu-error: Implement a more generic error reporting

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Thu, Jul 06, 2017 at 01:27:15PM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange" <address@hidden> writes:
>> 
>> > On Thu, Jul 06, 2017 at 08:15:54AM +0200, Markus Armbruster wrote:
>> >> Alistair Francis <address@hidden> writes:
>> >> 
>> >> > This patch converts the existing error_vreport() function into a generic
>> >> > qmesg_vreport() function that takes an enum describing the
>> >> > information to be reported.
>> >> >
>> >> > As part of this change a new qmesg_report() function is added as well 
>> >> > with the
>> >> > same capability.
>> >> >
>> >> > To maintain full compatibility the original error_report() function is
>> >> > maintained and no changes to the way errors are printed have been made.
>> >> > To improve access to the new informaiton and warning options wrapper 
>> >> > functions
>> >> > similar to error_report() have been added for warnings and information
>> >> > printing.
>> >> >
>> >> > Signed-off-by: Alistair Francis <address@hidden>
>> >
>> >> > diff --git a/util/qemu-error.c b/util/qemu-error.c
>> >> > index 1c5e35ecdb..63fdc0e174 100644
>> >> > --- a/util/qemu-error.c
>> >> > +++ b/util/qemu-error.c
>> >> > @@ -179,17 +179,29 @@ static void print_loc(void)
>> >> >  
>> >> >  bool enable_timestamp_msg;
>> >> >  /*
>> >> > - * Print an error message to current monitor if we have one, else to 
>> >> > stderr.
>> >> > + * Print a message to current monitor if we have one, else to stderr.
>> >> >   * Format arguments like vsprintf().  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.
>> >> >   */
>> >> > -void error_vreport(const char *fmt, va_list ap)
>> >> > +void qmsg_vreport(report_type type, const char *fmt, va_list ap)
>> >> >  {
>> >> >      GTimeVal tv;
>> >> >      gchar *timestr;
>> >> >  
>> >> > +    switch (type) {
>> >> > +    case REPORT_TYPE_ERROR:
>> >> > +        /* To maintain compatibility we don't add anything here */
>> >> 
>> >> I feel the comment isn't going to be useful in the future.  Let's drop
>> >> it.
>> >
>> > Do we really need to care about compatibility of the precise way we output
>> > error messages. It has never been something we call a "stable API", as we
>> > don't guarantee error message text will remain the same across releases. So
>> > anyone relying on scraping QEMU stderr to match some error message has 
>> > always
>> > been liable to break.
>> >
>> > IOW, just add an "error: " prefix to the text
>> 
>> I agree the error message format isn't ABI.
>> 
>> But what would adding "error: " buy us?
>
> It would clearly distinguish errors from any other output on stderr, which
> may not be error related (for example SPICE commonly pollutes stderr with
> lots of messages).

Changing the current error message format

    <TIMESTAMP><PROGNAME>:<LOCATION><MSG>

to

    <TIMESTAMP><PROGNAME>:<LOCATION>error: <MSG>

makes recognizing error messages a bit easier, but it also makes them
even longer.  Can't we make do with recognizing <PROGNAME>:?

If SPICE babbles to stderr, it needs a gag.