Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Mon, Jul 17, 2017 at 10:58 AM, Daniel P. Berrange
wrote:
> On Mon, Jul 17, 2017 at 08:54:18AM +0200, Ladi Prosek wrote:
>> On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange
>> wrote:
>> > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi 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.
>> >>
>> >> 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);
>> >
>> > You could get the same thing by turning virtio_Error into a macro with
>> > a few games. Rename the current method to virtio_error_impl() and then
>> > define:
>> >
>> > #define virtio_error(dev, fmt, ...) \
>> > virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)
>>
>> Neat! I think I'll stick with a function though. This doesn't allocate
>> but it adds a little bit of code to each call site which has the
>> potential of slowing down the fast no-error path (I have no data, just
>> the general keeping-the-code-compact-is-good principle). Holler if you
>> disagree!
>
> IMHO that would be uneccessary optimization, particular since this is in
> the error scenario and so is not performance critical to normal operation.
Yeah, what I mean is that code getting bigger may have negative impact
even if it doesn't execute - it takes up space in caches and such.
Maybe it's an overkill but some of this common virtio code is pretty
low-level and every cache line counts. Actually, I'm tempted to wrap
the error conditions in virtio.c in unlikely() so the compiler knows
it's not part of normal operation. Thanks!
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Mon, Jul 17, 2017 at 08:54:18AM +0200, Ladi Prosek wrote:
> On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange
> wrote:
> > On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi 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.
> >>
> >> 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);
> >
> > You could get the same thing by turning virtio_Error into a macro with
> > a few games. Rename the current method to virtio_error_impl() and then
> > define:
> >
> > #define virtio_error(dev, fmt, ...) \
> > virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)
>
> Neat! I think I'll stick with a function though. This doesn't allocate
> but it adds a little bit of code to each call site which has the
> potential of slowing down the fast no-error path (I have no data, just
> the general keeping-the-code-compact-is-good principle). Holler if you
> disagree!
IMHO that would be uneccessary optimization, particular since this is in
the error scenario and so is not performance critical to normal operation.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Fri, Jul 14, 2017 at 12:41 PM, Daniel P. Berrange
wrote:
> On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi 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.
>>
>> 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);
>
> You could get the same thing by turning virtio_Error into a macro with
> a few games. Rename the current method to virtio_error_impl() and then
> define:
>
> #define virtio_error(dev, fmt, ...) \
> virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)
Neat! I think I'll stick with a function though. This doesn't allocate
but it adds a little bit of code to each call site which has the
potential of slowing down the fast no-error path (I have no data, just
the general keeping-the-code-compact-is-good principle). Holler if you
disagree!
> Regards,
> Daniel
> --
> |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Sat, Jul 15, 2017 at 7:50 AM, Markus Armbruster wrote:
> Stefan Hajnoczi 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
>>> 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.
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
Stefan Hajnoczi 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 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.
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Thu, Jul 13, 2017 at 02:32:06PM +0100, Stefan Hajnoczi 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.
>
> 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);
You could get the same thing by turning virtio_Error into a macro with
a few games. Rename the current method to virtio_error_impl() and then
define:
#define virtio_error(dev, fmt, ...) \
virtio_error_impl(dev, "%s: " fmt, DEVICE(dev)->id, __VA_ARGS__)
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Thu, Jul 13, 2017 at 03:48:02PM +0200, Ladi Prosek wrote:
> On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi 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
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
On Thu, Jul 13, 2017 at 3:32 PM, Stefan Hajnoczi 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.
> Stefan
Re: [Qemu-devel] [PATCH v2 1/8] qemu-error: introduce error_report_nolf
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.
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
Stefan
signature.asc
Description: PGP signature
