[Qemu-devel] [PATCH v2 0/2] NBD fixes before softfreeze

[Qemu-devel] [PATCH v2 0/2] NBD fixes before softfreeze

[Eric Blake]

Paolo's inclusion of my NBD series in his miscellaneous tree
ended up with a couple of bugs that were pointed out in the
thread after the pull request was first created; these are the
fixes.  They are small enough that I don't mind using maintainer
status to bundle them up as a PULL request for NBD sometime on
Monday (in order to make soft freeze), whether or not they've
been reviewed by then; but of course, review is appreciated.

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01983.html
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01981.html

since v1:
- more defines and comments to make patch 2 more legible [John]

001/2:[----] [--] 'nbd: Trace client command being sent'
002/2:[0026] [FC] 'nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients'

Eric Blake (2):
  nbd: Trace client command being sent
  nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

 nbd/nbd-internal.h |  8 ++++++--
 nbd/client.c       |  3 ++-
 nbd/server.c       | 18 ++++++++++--------
 nbd/trace-events   |  2 +-
 4 files changed, 19 insertions(+), 12 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH v2 1/2] nbd: Trace client command being sent

[Eric Blake]

Make the client trace slightly more legible by including the name
of the command being sent.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 nbd/client.c     | 3 ++-
 nbd/trace-events | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index c3ee9f36b1..4c12fffcbf 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -902,7 +902,8 @@ ssize_t nbd_send_request(QIOChannel *ioc, NBDRequest *request)
     uint8_t buf[NBD_REQUEST_SIZE];

     trace_nbd_send_request(request->from, request->len, request->handle,
-                           request->flags, request->type);
+                           request->flags, request->type,
+                           nbd_cmd_lookup(request->type));

     stl_be_p(buf, NBD_REQUEST_MAGIC);
     stw_be_p(buf + 4, request->flags);
diff --git a/nbd/trace-events b/nbd/trace-events
index f5024d85a1..d7c7746ea8 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -28,7 +28,7 @@ nbd_client_loop(void) "Doing NBD loop"
 nbd_client_loop_ret(int ret, const char *error) "NBD loop returned %d: %s"
 nbd_client_clear_queue(void) "Clearing NBD queue"
 nbd_client_clear_socket(void) "Clearing NBD socket"
-nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = %" PRIx16 ", .type = %" PRIu16 " }"
+nbd_send_request(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name) "Sending request to server: { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = %" PRIx16 ", .type = %" PRIu16 " (%s) }"
 nbd_receive_reply(uint32_t magic, int32_t error, uint64_t handle) "Got reply: { magic = 0x%" PRIx32 ", .error = % " PRId32 ", handle = %" PRIu64" }"

 # nbd/server.c
-- 
2.13.3

[Qemu-devel] [PATCH v2 2/2] nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

[Eric Blake]

A typo in commit 23e099c set the size of buf[] used in response
to NBD_OPT_EXPORT_NAME according to the length needed for old-style
negotiation (4 bytes of flag information) instead of the intended
2 bytes used in new style.  If the client doesn't enable
NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many,
and is then out of sync in response to the client's next command
(the bug is masked when modern qemu is the client, since we enable
the no zeroes flag).

While touching this code, add some more defines to nbd_internal.h
rather than having quite so many magic numbers in the .c; also,
use "" initialization rather than memset(), and tweak the oldstyle
negotiation to better match the spec description of the layout
(since the spec is big-endian, skipping two bytes as 0 followed by
writing a 2-byte flag is the same as writing a zero-extended 4-byte
flag), to make it a bit easier to follow compared to the spec.

[checkpatch.pl has some false positives in the comments]

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: defines rather than magic numbers [John]
---
 nbd/nbd-internal.h |  8 ++++++--
 nbd/server.c       | 18 ++++++++++--------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 4065bc68ac..396ddb4d3e 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -38,9 +38,13 @@
  */

 /* Size of all NBD_OPT_*, without payload */
-#define NBD_REQUEST_SIZE        (4 + 2 + 2 + 8 + 8 + 4)
+#define NBD_REQUEST_SIZE            (4 + 2 + 2 + 8 + 8 + 4)
 /* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
-#define NBD_REPLY_SIZE          (4 + 4 + 8)
+#define NBD_REPLY_SIZE              (4 + 4 + 8)
+/* Size of reply to NBD_OPT_EXPORT_NAME */
+#define NBD_REPLY_EXPORT_NAME_SIZE  (8 + 2 + 124)
+/* Size of oldstyle negotiation */
+#define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124)

 #define NBD_REQUEST_MAGIC       0x25609513
 #define NBD_REPLY_MAGIC         0x67446698
diff --git a/nbd/server.c b/nbd/server.c
index 49ed57455c..82a78bf439 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -283,12 +283,16 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
                                             Error **errp)
 {
     char name[NBD_MAX_NAME_SIZE + 1];
-    char buf[8 + 4 + 124] = "";
+    char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
     size_t len;
     int ret;

     /* Client sends:
         [20 ..  xx]   export name (length bytes)
+       Server replies:
+        [ 0 ..   7]   size
+        [ 8 ..   9]   export flags
+        [10 .. 133]   reserved     (0) [unless no_zeroes]
      */
     trace_nbd_negotiate_handle_export_name();
     if (length >= sizeof(name)) {
@@ -800,22 +804,21 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
  */
 static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
 {
-    char buf[8 + 8 + 8 + 128];
+    char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";
     int ret;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
                               NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
                               NBD_FLAG_SEND_WRITE_ZEROES);
     bool oldStyle;

-    /* Old style negotiation header without options
+    /* Old style negotiation header, no room for options
         [ 0 ..   7]   passwd       ("NBDMAGIC")
         [ 8 ..  15]   magic        (NBD_CLIENT_MAGIC)
         [16 ..  23]   size
-        [24 ..  25]   server flags (0)
-        [26 ..  27]   export flags
+        [24 ..  27]   export flags (zero-extended)
         [28 .. 151]   reserved     (0)

-       New style negotiation header with options
+       New style negotiation header, client can send options
         [ 0 ..   7]   passwd       ("NBDMAGIC")
         [ 8 ..  15]   magic        (NBD_OPTS_MAGIC)
         [16 ..  17]   server flags (0)
@@ -825,7 +828,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
     qio_channel_set_blocking(client->ioc, false, NULL);

     trace_nbd_negotiate_begin();
-    memset(buf, 0, sizeof(buf));
     memcpy(buf, "NBDMAGIC", 8);

     oldStyle = client->exp != NULL && !client->tlscreds;
@@ -834,7 +836,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
                                       client->exp->nbdflags | myflags);
         stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
         stq_be_p(buf + 16, client->exp->size);
-        stw_be_p(buf + 26, client->exp->nbdflags | myflags);
+        stl_be_p(buf + 24, client->exp->nbdflags | myflags);

         if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) {
             error_prepend(errp, "write failed: ");
-- 
2.13.3

Re: [Qemu-devel] [PATCH v2 2/2] nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

[John Snow]

On 07/17/2017 03:26 PM, Eric Blake wrote:
> A typo in commit 23e099c set the size of buf[] used in response
> to NBD_OPT_EXPORT_NAME according to the length needed for old-style
> negotiation (4 bytes of flag information) instead of the intended
> 2 bytes used in new style.  If the client doesn't enable
> NBD_FLAG_C_NO_ZEROES, then the server sends two bytes too many,
> and is then out of sync in response to the client's next command
> (the bug is masked when modern qemu is the client, since we enable
> the no zeroes flag).
> 
> While touching this code, add some more defines to nbd_internal.h
> rather than having quite so many magic numbers in the .c; also,
> use "" initialization rather than memset(), and tweak the oldstyle
> negotiation to better match the spec description of the layout
> (since the spec is big-endian, skipping two bytes as 0 followed by
> writing a 2-byte flag is the same as writing a zero-extended 4-byte
> flag), to make it a bit easier to follow compared to the spec.
> 
> [checkpatch.pl has some false positives in the comments]
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: defines rather than magic numbers [John]
> ---
>  nbd/nbd-internal.h |  8 ++++++--
>  nbd/server.c       | 18 ++++++++++--------
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> index 4065bc68ac..396ddb4d3e 100644
> --- a/nbd/nbd-internal.h
> +++ b/nbd/nbd-internal.h
> @@ -38,9 +38,13 @@
>   */
> 
>  /* Size of all NBD_OPT_*, without payload */
> -#define NBD_REQUEST_SIZE        (4 + 2 + 2 + 8 + 8 + 4)
> +#define NBD_REQUEST_SIZE            (4 + 2 + 2 + 8 + 8 + 4)
>  /* Size of all NBD_REP_* sent in answer to most NBD_OPT_*, without payload */
> -#define NBD_REPLY_SIZE          (4 + 4 + 8)
> +#define NBD_REPLY_SIZE              (4 + 4 + 8)
> +/* Size of reply to NBD_OPT_EXPORT_NAME */
> +#define NBD_REPLY_EXPORT_NAME_SIZE  (8 + 2 + 124)
> +/* Size of oldstyle negotiation */
> +#define NBD_OLDSTYLE_NEGOTIATE_SIZE (8 + 8 + 8 + 4 + 124)
> 
>  #define NBD_REQUEST_MAGIC       0x25609513
>  #define NBD_REPLY_MAGIC         0x67446698
> diff --git a/nbd/server.c b/nbd/server.c
> index 49ed57455c..82a78bf439 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -283,12 +283,16 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
>                                              Error **errp)
>  {
>      char name[NBD_MAX_NAME_SIZE + 1];
> -    char buf[8 + 4 + 124] = "";
> +    char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
>      size_t len;
>      int ret;
> 
>      /* Client sends:
>          [20 ..  xx]   export name (length bytes)
> +       Server replies:
> +        [ 0 ..   7]   size
> +        [ 8 ..   9]   export flags
> +        [10 .. 133]   reserved     (0) [unless no_zeroes]

Alright, there's the 8, 2, 124.

>       */
>      trace_nbd_negotiate_handle_export_name();
>      if (length >= sizeof(name)) {
> @@ -800,22 +804,21 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>   */
>  static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>  {
> -    char buf[8 + 8 + 8 + 128];
> +    char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";

huh, TIL.

>      int ret;
>      const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>                                NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
>                                NBD_FLAG_SEND_WRITE_ZEROES);
>      bool oldStyle;
> 
> -    /* Old style negotiation header without options
> +    /* Old style negotiation header, no room for options
>          [ 0 ..   7]   passwd       ("NBDMAGIC")
>          [ 8 ..  15]   magic        (NBD_CLIENT_MAGIC)
>          [16 ..  23]   size
> -        [24 ..  25]   server flags (0)
> -        [26 ..  27]   export flags
> +        [24 ..  27]   export flags (zero-extended)
>          [28 .. 151]   reserved     (0)

And there's the 8, 8, 8, 4, 124.

> 
> -       New style negotiation header with options
> +       New style negotiation header, client can send options
>          [ 0 ..   7]   passwd       ("NBDMAGIC")
>          [ 8 ..  15]   magic        (NBD_OPTS_MAGIC)
>          [16 ..  17]   server flags (0)
> @@ -825,7 +828,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>      qio_channel_set_blocking(client->ioc, false, NULL);
> 
>      trace_nbd_negotiate_begin();
> -    memset(buf, 0, sizeof(buf));
>      memcpy(buf, "NBDMAGIC", 8);
> 
>      oldStyle = client->exp != NULL && !client->tlscreds;
> @@ -834,7 +836,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>                                        client->exp->nbdflags | myflags);
>          stq_be_p(buf + 8, NBD_CLIENT_MAGIC);
>          stq_be_p(buf + 16, client->exp->size);
> -        stw_be_p(buf + 26, client->exp->nbdflags | myflags);
> +        stl_be_p(buf + 24, client->exp->nbdflags | myflags);

Makes sense to me.

> 
>          if (nbd_write(client->ioc, buf, sizeof(buf), errp) < 0) {
>              error_prepend(errp, "write failed: ");
> 

Thanks,

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/2] NBD fixes before softfreeze

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On 07/17/2017 02:26 PM, Eric Blake wrote:
> Paolo's inclusion of my NBD series in his miscellaneous tree
> ended up with a couple of bugs that were pointed out in the
> thread after the pull request was first created; these are the
> fixes.  They are small enough that I don't mind using maintainer
> status to bundle them up as a PULL request for NBD sometime on
> Monday (in order to make soft freeze), whether or not they've
> been reviewed by then; but of course, review is appreciated.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01983.html
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01981.html
> 
> since v1:
> - more defines and comments to make patch 2 more legible [John]
> 
> 001/2:[----] [--] 'nbd: Trace client command being sent'
> 002/2:[0026] [FC] 'nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients'
> 
> Eric Blake (2):
>   nbd: Trace client command being sent
>   nbd: Fix server reply to NBD_OPT_EXPORT_NAME of older clients

Thanks; queued on my NBD branch:
git://repo.or.cz/qemu/ericb.git nbd

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]