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

[Ladi Prosek]

On Mon, Jul 17, 2017 at 10:58 AM, Daniel P. Berrange
<address@hidden> 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
>> <address@hidden> 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 :|