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

[Eric Blake]

On 06/21/2017 10:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> Separate case when client sent NBD_OPT_ABORT from other errors.

Commit messages are best written in imperative tense (you can supply an
implicit "apply this patch in order to" prefix in front of the message,
and it should still generally read well); and that doesn't mix well with
past-tense descriptions.  I might have written:

Separate the case when a client sends NBD_OPT_ABORT from all 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.

This particular case is not actually an error - it honestly follows the
NBD protocol.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  nbd/server.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 

> -
> -/* 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:
> + * -errno  on error
> + * 0       on successful negotiation
> + * 1       if client sent NBD_OPT_ABORT, i.e. on legal disconnect

"legal" often comes with connotations about law (and I don't want to get
mired in an open-source law discussion); I'd stick with "valid".

> + */
>  static int nbd_negotiate_options(NBDClient *client)
>  {
>      uint32_t flags;
> @@ -459,7 +463,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;

Note: the reason we don't try to send an NBD_REP_ACK here (a guest that
forgot to start the TLS handshake) is because we don't want to ever
return NBD_REP_ACK when we are going to require TLS.  It's a bit odd
that we don't reply here, but DO reply to all other NBD_OPT_ABORT, but I
don't think it makes us any less compliant to the spec.

>                  }
>                  break;
>              }
> @@ -477,7 +481,7 @@ static int nbd_negotiate_options(NBDClient *client)
>                   * disconnecting, but that we must also tolerate
>                   * guests that don't wait for our reply. */
>                  nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, 
> clientflags);
> -                return -EINVAL;
> +                return 1;

If anything, we may want (as a separate patch) to add a comment to our
TLS behavior as to why we don't reply with NBD_REP_ACK.  But doesn't
affect this patch.

>  
>              case NBD_OPT_EXPORT_NAME:
>                  return nbd_negotiate_handle_export_name(client, length);
> @@ -533,6 +537,12 @@ static int nbd_negotiate_options(NBDClient *client)
>      }
>  }
>  
> +/* nbd_negotiate
> + * Return:
> + * -errno  on error
> + * 0       on successful negotiation
> + * 1       if client sent NBD_OPT_ABORT, i.e. on legal disconnect

Again on the wording.  With that touched up,
Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature