Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: use errp instead of LOG

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Move to modern errp scheme from just LOGging errors.
>> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>  nbd/server.c | 268 
>> +++++++++++++++++++++++++++++++++++------------------------
>>  1 file changed, 161 insertions(+), 107 deletions(-)
>
> Unfortunately longer, but I'm okay with that (and we already did it
> client-side, so this is just a case of a patch series being spread out
> over time).
>
>
>>  
>>  /* Send an error reply.
>>   * Return -errno on error, 0 on success. */
>> -static int GCC_FMT_ATTR(4, 5)
>> +static int GCC_FMT_ATTR(5, 6)
>>  nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
>> -                           uint32_t opt, const char *fmt, ...)
>> +                           uint32_t opt, Error **errp, const char *fmt, ...)
>>  {
>
> Markus, this violates our usual idiom of errp last in code that is not
> directly operating on an Error (the actual Error implementation in
> error.c being the main obvious exception that lists errp first), but I
> don't see any better approach. Do you have any thoughts on it?

The convention to put Error **errp last comes from GLib's GError.
Here's what it has to say about combining GError with variable
arguments:

    The last argument of a function that returns an error should be a
    location where a GError can be placed (i.e. "GError** error"). If
    GError is used with varargs, the GError** should be the last
    argument before the "...".

https://developer.gnome.org/glib/unstable/glib-Error-Reporting.html

Separating the format string from the variable arguments is ugly,
though.  I can't find examples of this ugliness in /usr/include/glib.  I
can find a few for Vladimir's solution:

* g_output_stream_printf() in gio/goutputstream.h: format string
  followed by arguments

* g_initable_new() in gio/ginitable.h: key name followed by key value,
  ..., NULL

* g_subprocess_new() in gio/gsubprocess.h: argv0, ..., NULL

* g_subprocess_launcher_spawn() in gio/gsubprocesslauncher.h: argv0,
  ..., NULL

* g_markup_collect_attributes() in gmarkup.h: attribute name followed by
  attribute value, ..., NULL

Let's follow this precedence.  In short, Vladimir's patch is okay as is.

[...]