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

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote:
>> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> 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.

Concur.

When malloc() starts to fail or crash, the process is basically screwed.
This should be rare.  Keeping error paths simple to improve the chances
of a useful error message getting through before the process dies is a
laudable idea anyway.  But:

* When timestamps are enabled, error_vreport() already allocates.

* When error messages go to the monitor, monitor_vprintf() already
  allocates().

We could get rid of both allocations with some effort.  Wouldn't affect
the interface.  Until we do, avoiding another small allocation is
unlikely to help.

> Performance isn't a big concern for printing error messages.  That's why
> I suggest a single error_report() call instead.