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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

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 <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 268 +++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 161 insertions(+), 107 deletions(-)

> -static coroutine_fn int nbd_negotiate(NBDClient *client)
> +static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>  {
>      char buf[8 + 8 + 8 + 128];
>      int ret;

Now that you have this,

>     qio_channel_set_blocking(client->ioc, false, NULL);
> 

you should probably pass errp through to qio_channel_set_blocking and
return early if that fails (either here or in one of the followup
patches) [hmm, I note that most callers of qio_channel_set_blocking()
ignore errors - outside of NBD, so that's a separate cleanup series]


> @@ -591,21 +617,23 @@ static coroutine_fn int nbd_negotiate(NBDClient *client)
>  
>      if (oldStyle) {
>          if (client->tlscreds) {
> -            TRACE("TLS cannot be enabled with oldstyle protocol");
> +            error_setg(errp, "TLS cannot be enabled with oldstyle protocol");
>              return -EINVAL;

This code is unreachable, since earlier we have:

>     oldStyle = client->exp != NULL && !client->tlscreds;

You could just as easily delete the conditional.  In fact, our code has:

if (oldStyle) {
    do stuff
} else {
    do stuff
}

if (oldStyle) {
    do stuff
} else {
    do stuff
}

and it would be simple to merge that into a single if statement.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]