Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf

[Stefan Hajnoczi]

On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote:
> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <address@hidden> wrote:
> > On Thu, Jul 13, 2017 at 01:02:30PM +0200, Ladi Prosek wrote:
> >> +/*
> >> + * Print an error message to current monitor if we have one, else to 
> >> stderr.
> >> + * Format arguments like sprintf().  The resulting message should be a
> >> + * single phrase, with no trailing punctuation.  The no-LF version allows
> >> + * additional text to be appended with error_printf() or error_vprintf().
> >> + * Make sure to always close with a newline after all text is printed.
> >> + * Prepends the current location.
> >> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> >> + */
> >> +void error_report_nolf(const char *fmt, ...)
> >> +{
> >> +    va_list ap;
> >> +
> >> +    va_start(ap, fmt);
> >> +    error_vreport_nolf(fmt, ap);
> >> +    va_end(ap);
> >>  }
> >
> > Each call to this function prepends the timestamp, so it cannot really
> > be used for a sequence of prints in a single line.
> 
> True, the _nolf means "does not append \n" rather than "can be called
> repeatedly". You would use error_printf() / error_vprintf() for
> subsequent prints, as mentioned in the comment above this function.
> 
> > It's a little ugly but I expected something along the lines of
> > g_strdup_vprintf() in virtio_error():
> >
> >   char *msg;
> >
> >   va_start(ap, fmt);
> >   msg = g_strdup_vprintf(fmt, ap);
> >   va_end(ap);
> >
> >   error_report("%s: %s", DEVICE(vdev)->id, msg);
> >
> >   g_free(msg);
> >
> > https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup-vprintf
> 
> I wanted to avoid the memory allocation, that's all. Not for
> performance, obviously, but to increase the chances that it will come
> through and always look the same if the process is under memory
> pressure, the heap corrupted or otherwise in a bad state.
> 
> Both approaches look about the same ugly to me but I can certainly
> change it to the one you're suggesting.

The chance that error_report_nolf() will be used incorrectly is high.
Performance isn't a big concern for printing error messages.  That's why
I suggest a single error_report() call instead.

Stefan

signature.asc
Description: PGP signature