Re: [Qemu-devel] [PATCH 13/18] nbd: Support shorter handshake

[Alex Bligh]

On 8 Apr 2016, at 23:05, Eric Blake <address@hidden> wrote:

> The NBD Protocol allows the server and client to mutually agree
> on a shorter handshake (omit the 124 bytes of reserved 0), via
> the server advertising NBD_FLAG_NO_ZEROES and the client
> acknowledging with NBD_FLAG_C_NO_ZEROES (only possible in
> newstyle, whether or not it is fixed newstyle).  It doesn't
> shave much off the wire, but we might as well implement it.
> 
> Signed-off-by: Eric Blake <address@hidden>


Reviewed-by: Alex Bligh <address@hidden>

thanks - that was annoying me.

> ---
> include/block/nbd.h |  6 ++++--
> nbd/client.c        |  8 +++++++-
> nbd/server.c        | 15 +++++++++++----
> 3 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 155196e..35c0ea3 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -73,11 +73,13 @@ typedef struct nbd_reply nbd_reply;
> 
> /* New-style handshake (global) flags, sent from server to client, and
>    control what will happen during handshake phase. */
> -#define NBD_FLAG_FIXED_NEWSTYLE     (1 << 0)    /* Fixed newstyle protocol. 
> */
> +#define NBD_FLAG_FIXED_NEWSTYLE   (1 << 0) /* Fixed newstyle protocol. */
> +#define NBD_FLAG_NO_ZEROES        (1 << 1) /* End handshake without zeroes. 
> */
> 
> /* New-style client flags, sent from client to server to control what happens
>    during handshake phase. */
> -#define NBD_FLAG_C_FIXED_NEWSTYLE   (1 << 0)    /* Fixed newstyle protocol. 
> */
> +#define NBD_FLAG_C_FIXED_NEWSTYLE (1 << 0) /* Fixed newstyle protocol. */
> +#define NBD_FLAG_C_NO_ZEROES      (1 << 1) /* End handshake without zeroes. 
> */
> 
> /* Reply types. */
> #define NBD_REP_ACK             (1)             /* Data sending finished. */
> diff --git a/nbd/client.c b/nbd/client.c
> index d4e37d5..507ddc1 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -409,6 +409,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>     char buf[256];
>     uint64_t magic, s;
>     int rc;
> +    bool zeroes = true;
> 
>     TRACE("Receiving negotiation tlscreds=%p hostname=%s.",
>           tlscreds, hostname ? hostname : "<null>");
> @@ -475,6 +476,11 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>             TRACE("Server supports fixed new style");
>             clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
>         }
> +        if (globalflags & NBD_FLAG_NO_ZEROES) {
> +            zeroes = false;
> +            TRACE("Server supports no zeroes");
> +            clientflags |= NBD_FLAG_C_NO_ZEROES;
> +        }
>         /* client requested flags */
>         clientflags = cpu_to_be32(clientflags);
>         if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
> @@ -558,7 +564,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
> *name, uint32_t *flags,
>         goto fail;
>     }
> 
> -    if (drop_sync(ioc, 124) != 124) {
> +    if (zeroes && drop_sync(ioc, 124) != 124) {
>         error_setg(errp, "Failed to read reserved block");
>         goto fail;
>     }
> diff --git a/nbd/server.c b/nbd/server.c
> index 69724c9..379df8c 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -78,6 +78,7 @@ struct NBDClient {
>     int refcount;
>     void (*close)(NBDClient *client);
> 
> +    bool no_zeroes;
>     NBDExport *exp;
>     QCryptoTLSCreds *tlscreds;
>     char *tlsaclname;
> @@ -396,6 +397,11 @@ static int nbd_negotiate_options(NBDClient *client)
>         fixedNewstyle = true;
>         flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
>     }
> +    if (flags & NBD_FLAG_C_NO_ZEROES) {
> +        TRACE("Client supports no zeroes at handshake end");
> +        client->no_zeroes = true;
> +        flags &= ~NBD_FLAG_C_NO_ZEROES;
> +    }
>     if (flags != 0) {
>         TRACE("Unknown client flags 0x%" PRIx32 " received", flags);
>         return -EIO;
> @@ -527,6 +533,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
>     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>                               NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
>     bool oldStyle;
> +    size_t len;
> 
>     /* Old style negotiation header without options
>         [ 0 ..   7]   passwd       ("NBDMAGIC")
> @@ -543,7 +550,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
>         ....options sent....
>         [18 ..  25]   size
>         [26 ..  27]   export flags
> -        [28 .. 151]   reserved     (0)
> +        [28 .. 151]   reserved     (0, omit if no_zeroes)
>      */
> 
>     qio_channel_set_blocking(client->ioc, false, NULL);
> @@ -560,7 +567,7 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
>         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
>     } else {
>         stq_be_p(buf + 8, NBD_OPTS_MAGIC);
> -        stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE);
> +        stw_be_p(buf + 16, NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES);
>     }
> 
>     if (oldStyle) {
> @@ -585,8 +592,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData 
> *data)
> 
>         stq_be_p(buf + 18, client->exp->size);
>         stw_be_p(buf + 26, client->exp->nbdflags | myflags);
> -        if (nbd_negotiate_write(client->ioc, buf + 18, sizeof(buf) - 18) !=
> -            sizeof(buf) - 18) {
> +        len = client->no_zeroes ? 10 : sizeof(buf) - 18;
> +        if (nbd_negotiate_write(client->ioc, buf + 18, len) != len) {
>             LOG("write failed");
>             goto fail;
>         }
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh