Re: [Qemu-devel] [PATCH 14/19] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

[Eric Blake]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Separate case when client sent NBD_OPT_ABORT from other errors.
> It will be needed for the following patch, where errors will be
> reported.
> Considered case is not actually the error - it honestly follows NBD
> protocol. Therefore it should not be reported like an error.
> -EPIPE case means client not read server reply on NBD_OPT_ABORT,
> which is also OK.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  nbd/server.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 30dfb81a5c..0e53d3dd91 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -369,9 +369,13 @@ static QIOChannel 
> *nbd_negotiate_handle_starttls(NBDClient *client,
>      return QIO_CHANNEL(tioc);
>  }
>  
> -
> -/* Process all NBD_OPT_* client option commands.
> - * Return -errno on error, 0 on success. */
> +/* nbd_negotiate_options
> + * Process all NBD_OPT_* client option commands.
> + * Return:
> + * < 0 on error

Do you want to be specific that this is a negative errno value, or is it
just any negative value with no correlation to errno?

> + * 0   on successful negotiation
> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> + */
>  static int nbd_negotiate_options(NBDClient *client)
>  {
>      int ret;
> @@ -483,7 +487,7 @@ static int nbd_negotiate_options(NBDClient *client)
>                  }
>                  /* Let the client keep trying, unless they asked to quit */
>                  if (clientflags == NBD_OPT_ABORT) {
> -                    return -EINVAL;
> +                    return 1;
>                  }
>                  break;
>              }
> @@ -502,7 +506,7 @@ static int nbd_negotiate_options(NBDClient *client)
>                   * guests that don't wait for our reply. */
>                  ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
>                                               clientflags);
> -                return ret < 0 ? ret : -EINVAL;
> +                return ret < 0 && ret != -EPIPE ? ret : 1;

This should just be 'return 1;', which means you don't need to capture
and check 'ret'.

>  
>              case NBD_OPT_EXPORT_NAME:
>                  return nbd_negotiate_handle_export_name(client, length);
> @@ -560,6 +564,12 @@ static int nbd_negotiate_options(NBDClient *client)
>      }
>  }
>  
> +/* nbd_negotiate
> + * Return:
> + * < 0 on error

Again, if this is reliably a negative errno, specifically document that.

> + * 0   on successful negotiation
> + * 1   if client sent NBD_OPT_ABORT, i.e. on legal disconnect
> + */
>  static coroutine_fn int nbd_negotiate(NBDClient *client)
>  {
>      char buf[8 + 8 + 8 + 128];
> 

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

signature.asc
Description: OpenPGP digital signature