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

[Eric Blake]

On 04/09/2016 04:42 AM, Alex Bligh wrote:
> 
> On 8 Apr 2016, at 23:05, Eric Blake  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 
> 
> 
> Reviewed-by: Alex Bligh 
> 
> thanks - that was annoying me.
> 

It turns out that doing this also made the NBD_OPT_GO patches easier :)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Alex Bligh]

On 8 Apr 2016, at 23:05, Eric Blake  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 


Reviewed-by: Alex Bligh 

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 : "");
> @@ -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, , 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 

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

[Eric Blake]

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 
---
 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 : "");
@@ -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, , 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 =