[v2,1/6] nbd/server: nbd_negotiate: return 1 on NBD_OPT_ABORT

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.

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

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 <vsementsov@virtuozzo.com>
> ---
>  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 <eblake@redhat.com>

On 06/29/2017 01:46 PM, Eric Blake wrote:
> 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 <vsementsov@virtuozzo.com>
>> ---
>>  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
> 

Phooey, I'm just now noticing (while trying to rebase my existing
patches on top of yours) that we have a conflict in semantics.  For
reference, my patch was posted here:

https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04535.html

where I picked the semantics:

+/* Process all NBD_OPT_* client option commands, during fixed newstyle
+ * negotiation. Return -errno on error, 0 on successful
NBD_OPT_EXPORT_NAME,
+ * and 1 on successful NBD_OPT_GO. */
+static int nbd_negotiate_options(NBDClient *client, uint16_t myflags)

But since I'm rebasing my stuff anyways, I'll come up with some other
way to combine the two semantics (perhaps by refactoring the response to
NBD_OPT_EXPORT_NAME to occur in nbd_negotiate_options() instead of in
the caller, so that returning 0 is always sufficient to mark that
newstyle negotiation is complete and the server is now in transmission
phase).