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

[Ladi Prosek]

On Sat, Jul 15, 2017 at 7:50 AM, Markus Armbruster <address@hidden> wrote:
> Stefan Hajnoczi <address@hidden> 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 <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.
>
> 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.

Makes sense. v3 will have what you guys suggest. Thanks!

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