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