Libguestfs - Aug 2018 - Re: [PATCH nbdkit v2] protocol: Implement NBD_OPT_GO.

There's no substantial difference over v1, I simply fixed a few
whitespace issues, moved one struct around and tidied up the comments.

Rich.

---
 src/connections.c | 233 +++++++++++++++++++++++++++++++++++++---------
 src/protocol.h    |  27 ++++--
 2 files changed, 209 insertions(+), 51 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index ba6e91d..4e9b191 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -75,7 +75,9 @@ struct connection {
   void **handles;
   size_t nr_handles;
 
+  uint32_t cflags;
   uint64_t exportsize;
+  uint16_t eflags;
   int readonly;
   int can_flush;
   int is_rotational;
@@ -572,6 +574,55 @@ send_newstyle_option_reply_exportname (struct connection
*conn,
   return 0;
 }
 
+static int
+send_newstyle_option_reply_info_export (struct connection *conn,
+                                        uint32_t option, uint32_t reply,
+                                        uint16_t info)
+{
+  struct fixed_new_option_reply fixed_new_option_reply;
+  struct fixed_new_option_reply_info_export export;
+
+  fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC);
+  fixed_new_option_reply.option = htobe32 (option);
+  fixed_new_option_reply.reply = htobe32 (reply);
+  fixed_new_option_reply.replylen = htobe32 (sizeof export);
+  export.info = htobe16 (info);
+  export.exportsize = htobe64 (conn->exportsize);
+  export.eflags = htobe16 (conn->eflags);
+
+  if (conn->send (conn,
+                  &fixed_new_option_reply,
+                  sizeof fixed_new_option_reply) == -1 ||
+      conn->send (conn, &export, sizeof export) == -1) {
+    nbdkit_error ("write: %m");
+    return -1;
+  }
+
+  return 0;
+}
+
+static int
+get_size_and_eflags (struct connection *conn)
+{
+  int64_t r;
+
+  r = backend->get_size (backend, conn);
+  if (r == -1)
+    return -1;
+  if (r < 0) {
+    nbdkit_error (".get_size function returned invalid value "
+                  "(%" PRIi64 ")", r);
+    return -1;
+  }
+  conn->exportsize = (uint64_t) r;
+
+  if (compute_eflags (conn, &conn->eflags) < 0)
+    return -1;
+
+  debug ("newstyle negotiation: flags: export 0x%x",
conn->eflags);
+  return 0;
+}
+
 static int
 _negotiate_handshake_newstyle_options (struct connection *conn)
 {
@@ -581,6 +632,7 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
   uint32_t option;
   uint32_t optlen;
   char data[MAX_OPTION_LENGTH+1];
+  struct new_handshake_finish handshake_finish;
 
   for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
     if (conn->recv (conn, &new_option, sizeof new_option) == -1) {
@@ -625,7 +677,8 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
       }
       /* Apart from printing it, ignore the export name. */
       data[optlen] = '\0';
-      debug ("newstyle negotiation: client requested export '%s'
(ignored)",
+      debug ("newstyle negotiation: NBD_OPT_EXPORT_NAME: "
+             "client requested export '%s' (ignored)",
              data);
       break;
 
@@ -701,6 +754,101 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
       }
       break;
 
+    case NBD_OPT_GO:
+      if (conn->recv (conn, data, optlen) == -1) {
+        nbdkit_error ("read: %m");
+        return -1;
+      }
+
+      if (optlen < 6) { /* 32 bit export length + 16 bit nr info */
+        debug ("newstyle negotiation: NBD_OPT_GO option length <
6");
+
+        if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
+            == -1)
+          return -1;
+        continue;
+      }
+
+      {
+        uint32_t exportnamelen;
+        uint16_t nrinfos;
+        uint16_t info;
+        size_t i;
+        CLEANUP_FREE char *requested_exportname = NULL;
+
+        /* Validate the name length and number of INFO requests. */
+        memcpy (&exportnamelen, &data[0], 4);
+        exportnamelen = be32toh (exportnamelen);
+        if (exportnamelen > optlen-6 /* NB optlen >= 6, see above */) {
+          debug ("newstyle negotiation: NBD_OPT_GO: export name too
long");
+          if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
+              == -1)
+            return -1;
+          continue;
+        }
+        memcpy (&nrinfos, &data[exportnamelen+4], 2);
+        nrinfos = be16toh (nrinfos);
+        if (optlen != 4 + exportnamelen + 2 + 2*nrinfos) {
+          debug ("newstyle negotiation: NBD_OPT_GO: "
+                 "number of information requests incorrect");
+          if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
+              == -1)
+            return -1;
+          continue;
+        }
+
+        /* As with NBD_OPT_EXPORT_NAME we print the export name and then
+         * ignore it.
+         */
+        requested_exportname = malloc (exportnamelen+1);
+        if (requested_exportname == NULL) {
+          nbdkit_error ("malloc: %m");
+          return -1;
+        }
+        memcpy (requested_exportname, &data[4], exportnamelen);
+        requested_exportname[exportnamelen] = '\0';
+        debug ("newstyle negotiation: NBD_OPT_GO: "
+               "client requested export '%s' (ignored)",
+               requested_exportname);
+
+        /* The spec is confusing, but it is required that we send back
+         * NBD_INFO_EXPORT, even if the client did not request it!
+         * qemu client in particular does not request this, but will
+         * fail if we don't send it.
+         */
+        if (get_size_and_eflags (conn) == -1)
+          return -1;
+        if (send_newstyle_option_reply_info_export (conn, option,
+                                                    NBD_REP_INFO,
+                                                    NBD_INFO_EXPORT) == -1)
+          return -1;
+
+        /* For now we ignore all other info requests (but we must
+         * ignore NBD_INFO_EXPORT if it was requested, because we
+         * replied already above).  Therefore this loop doesn't do
+         * much at the moment.
+         */
+        for (i = 0; i < nrinfos; ++i) {
+          memcpy (&info, &data[4+exportnamelen+2+i*2], 2);
+          info = be16toh (info);
+          switch (info) {
+          case NBD_INFO_EXPORT: /* ignore - reply sent above */ break;
+          default:
+            debug ("newstyle negotiation: NBD_OPT_GO: "
+                   "ignoring NBD_INFO_* request %u", (unsigned)
info);
+            break;
+          }
+        }
+      }
+
+      /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK
+       * or ERROR packet.
+       */
+      if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
+        return -1;
+
+      break;
+
     default:
       /* Unknown option. */
       if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1)
@@ -712,10 +860,10 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
     }
 
     /* Note, since it's not very clear from the protocol doc, that the
-     * client must send NBD_OPT_EXPORT_NAME last, and that ends option
-     * negotiation.
+     * client must send NBD_OPT_EXPORT_NAME or NBD_OPT_GO last, and
+     * that ends option negotiation.
      */
-    if (option == NBD_OPT_EXPORT_NAME)
+    if (option == NBD_OPT_EXPORT_NAME || option == NBD_OPT_GO)
       break;
   }
 
@@ -733,6 +881,38 @@ _negotiate_handshake_newstyle_options (struct connection
*conn)
     return -1;
   }
 
+  /* If the last option was NBD_OPT_GO then we're all done. */
+  if (option == NBD_OPT_GO)
+    ;
+  /* Else if the last option was NBD_OPT_EXPORT_NAME then we have to
+   * finish the handshake by sending handshake_finish.
+   */
+  else if (option == NBD_OPT_EXPORT_NAME) {
+    if (get_size_and_eflags (conn) == -1)
+      return -1;
+
+    memset (&handshake_finish, 0, sizeof handshake_finish);
+    handshake_finish.exportsize = htobe64 (conn->exportsize);
+    handshake_finish.eflags = htobe16 (conn->eflags);
+
+    if (conn->send (conn,
+                    &handshake_finish,
+                    (conn->cflags & NBD_FLAG_NO_ZEROES)
+                    ? offsetof (struct new_handshake_finish, zeroes)
+                    : sizeof handshake_finish) == -1) {
+      nbdkit_error ("write: %m");
+      return -1;
+    }
+  }
+  /* The client is buggy.  The last option must be NBD_OPT_GO or
+   * NBD_OPT_EXPORT_NAME.
+   */
+  else {
+    nbdkit_error ("client options list didn't finish with NBD_OPT_GO
"
+           "or NBD_OPT_EXPORT_NAME");
+    return -1;
+  }
+
   return 0;
 }
 
@@ -741,11 +921,6 @@ _negotiate_handshake_newstyle (struct connection *conn)
 {
   struct new_handshake handshake;
   uint16_t gflags;
-  uint32_t cflags;
-  struct new_handshake_finish handshake_finish;
-  int64_t r;
-  uint64_t exportsize;
-  uint16_t eflags;
 
   gflags = NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES;
 
@@ -761,15 +936,15 @@ _negotiate_handshake_newstyle (struct connection *conn)
   }
 
   /* Client now sends us its 32 bit flags word ... */
-  if (conn->recv (conn, &cflags, sizeof cflags) == -1) {
+  if (conn->recv (conn, &conn->cflags, 4) == -1) {
     nbdkit_error ("read: %m");
     return -1;
   }
-  cflags = be32toh (cflags);
+  conn->cflags = be32toh (conn->cflags);
   /* ... which we check for accuracy. */
-  debug ("newstyle negotiation: client flags: 0x%x", cflags);
-  if (cflags & ~gflags) {
-    nbdkit_error ("client requested unknown flags 0x%x", cflags);
+  debug ("newstyle negotiation: client flags: 0x%x",
conn->cflags);
+  if (conn->cflags & ~gflags) {
+    nbdkit_error ("client requested unknown flags 0x%x",
conn->cflags);
     return -1;
   }
 
@@ -777,36 +952,6 @@ _negotiate_handshake_newstyle (struct connection *conn)
   if (_negotiate_handshake_newstyle_options (conn) == -1)
     return -1;
 
-  /* Finish the newstyle handshake. */
-  r = backend->get_size (backend, conn);
-  if (r == -1)
-    return -1;
-  if (r < 0) {
-    nbdkit_error (".get_size function returned invalid value "
-                  "(%" PRIi64 ")", r);
-    return -1;
-  }
-  exportsize = (uint64_t) r;
-  conn->exportsize = exportsize;
-
-  if (compute_eflags (conn, &eflags) < 0)
-    return -1;
-
-  debug ("newstyle negotiation: flags: export 0x%x", eflags);
-
-  memset (&handshake_finish, 0, sizeof handshake_finish);
-  handshake_finish.exportsize = htobe64 (exportsize);
-  handshake_finish.eflags = htobe16 (eflags);
-
-  if (conn->send (conn,
-                  &handshake_finish,
-                  (cflags & NBD_FLAG_NO_ZEROES)
-                  ? offsetof (struct new_handshake_finish, zeroes)
-                  : sizeof handshake_finish) == -1) {
-    nbdkit_error ("write: %m");
-    return -1;
-  }
-
   return 0;
 }
 
diff --git a/src/protocol.h b/src/protocol.h
index f9ab5f2..792a905 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -80,13 +80,6 @@ struct fixed_new_option_reply {
 
 #define NBD_REP_MAGIC UINT64_C(0x3e889045565a9)
 
-/* New-style handshake server reply. */
-struct new_handshake_finish {
-  uint64_t exportsize;
-  uint16_t eflags;            /* per-export flags */
-  char zeroes[124];           /* must be sent as zero bytes */
-} __attribute__((packed));
-
 /* Global flags. */
 #define NBD_FLAG_FIXED_NEWSTYLE 1
 #define NBD_FLAG_NO_ZEROES      2
@@ -105,15 +98,35 @@ struct new_handshake_finish {
 #define NBD_OPT_ABORT        2
 #define NBD_OPT_LIST         3
 #define NBD_OPT_STARTTLS     5
+#define NBD_OPT_GO           7
 
 #define NBD_REP_ACK          1
 #define NBD_REP_SERVER       2
+#define NBD_REP_INFO         3
 #define NBD_REP_ERR_UNSUP    0x80000001
 #define NBD_REP_ERR_POLICY   0x80000002
 #define NBD_REP_ERR_INVALID  0x80000003
 #define NBD_REP_ERR_PLATFORM 0x80000004
 #define NBD_REP_ERR_TLS_REQD 0x80000005
 
+#define NBD_INFO_EXPORT      0
+
+/* NBD_INFO_EXPORT reply (follows fixed_new_option_reply). */
+struct fixed_new_option_reply_info_export {
+  uint16_t info;                /* NBD_INFO_EXPORT */
+  uint64_t exportsize;          /* size of export */
+  uint16_t eflags;              /* per-export flags */
+} __attribute__((packed));
+
+/* New-style handshake server reply when using NBD_OPT_EXPORT_NAME.
+ * Modern clients use NBD_OPT_GO instead of this.
+ */
+struct new_handshake_finish {
+  uint64_t exportsize;
+  uint16_t eflags;            /* per-export flags */
+  char zeroes[124];           /* must be sent as zero bytes */
+} __attribute__((packed));
+
 /* Request (client -> server). */
 struct request {
   uint32_t magic;               /* NBD_REQUEST_MAGIC. */
-- 
2.18.0

Actually I was wrong, there *is* one substantive change over v1, which
is this:
> +  /* The client is buggy.  The last option must be NBD_OPT_GO or
> +   * NBD_OPT_EXPORT_NAME.
> +   */
> +  else {
> +    nbdkit_error ("client options list didn't finish with
NBD_OPT_GO "
> +           "or NBD_OPT_EXPORT_NAME");
> +    return -1;
> +  }
> +
>    return 0;
Current behaviour in nbdkit is that if the last requested option is a
recognized option, but is not NBD_OPT_EXPORT_NAME then nbdkit will
send the handshake_finish struct and continue to transport mode.

However I don't think nbdkit is currently behaving correctly.

In any case the proposed behaviour (above) is that in this case we
will close the connection abruptly and record an error in the log.
(There's no way to return an error back to the client when we get into
this situation.)

I am not certain if this is correct or not, but I believe it is: the
NBD protocol says the only way to exit option mode and enter
transmission mode is:

  "Transmission mode can be entered (by the client sending
  NBD_OPT_EXPORT_NAME or by the server responding to an NBD_OPT_GO
  with NBD_REP_ACK)."

(The other two ways mentioned in that section are about terminating
the connection completely).

This also implies that the number of options must never be zero, which
the spec does say in passing:

  "At this point, we move on to option haggling, during which point
  the client can send one or (in fixed newstyle) more options to the
                      ~~~
  server."

The current nbdkit code assumes this, so we're OK there.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 08/06/2018 05:45 AM, Richard W.M. Jones wrote:
> ---
>   src/connections.c | 233 +++++++++++++++++++++++++++++++++++++---------
>   src/protocol.h    |  27 ++++--
>   2 files changed, 209 insertions(+), 51 deletions(-)
> 
> @@ -701,6 +754,101 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>         }
>         break;
>   
> +    case NBD_OPT_GO:
> +      if (conn->recv (conn, data, optlen) == -1) {
> +        nbdkit_error ("read: %m");
> +        return -1;
> +      }
> +
> +      if (optlen < 6) { /* 32 bit export length + 16 bit nr info */
> +        debug ("newstyle negotiation: NBD_OPT_GO option length <
6");
> +
> +        if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
> +            == -1)
> +          return -1;
> +        continue;
> +      }
> +
> +      {
> +        uint32_t exportnamelen;
> +        uint16_t nrinfos;
> +        uint16_t info;
> +        size_t i;
> +        CLEANUP_FREE char *requested_exportname = NULL;
> +
> +        /* Validate the name length and number of INFO requests. */
> +        memcpy (&exportnamelen, &data[0], 4);
> +        exportnamelen = be32toh (exportnamelen);
> +        if (exportnamelen > optlen-6 /* NB optlen >= 6, see above
*/) {
Spacing around '-'
> +          debug ("newstyle negotiation: NBD_OPT_GO: export name too
long");
> +          if (send_newstyle_option_reply (conn, option,
NBD_REP_ERR_INVALID)
> +              == -1)
> +            return -1;
> +          continue;
> +        }
> +        memcpy (&nrinfos, &data[exportnamelen+4], 2);
Spacing around '+'
> +        nrinfos = be16toh (nrinfos);
> +        if (optlen != 4 + exportnamelen + 2 + 2*nrinfos) {
Spacing around '*'
> +          debug ("newstyle negotiation: NBD_OPT_GO: "
> +                 "number of information requests incorrect");
> +          if (send_newstyle_option_reply (conn, option,
NBD_REP_ERR_INVALID)
> +              == -1)
> +            return -1;
> +          continue;
> +        }
> +
> +        /* As with NBD_OPT_EXPORT_NAME we print the export name and then
> +         * ignore it.
> +         */
> +        requested_exportname = malloc (exportnamelen+1);
Spacing around '+'
> +        if (requested_exportname == NULL) {
> +          nbdkit_error ("malloc: %m");
> +          return -1;
> +        }
> +        memcpy (requested_exportname, &data[4], exportnamelen);
> +        requested_exportname[exportnamelen] = '\0';
> +        debug ("newstyle negotiation: NBD_OPT_GO: "
> +               "client requested export '%s' (ignored)",
> +               requested_exportname);
> +
> +        /* The spec is confusing, but it is required that we send back
> +         * NBD_INFO_EXPORT, even if the client did not request it!
> +         * qemu client in particular does not request this, but will
> +         * fail if we don't send it.
> +         */
> +        if (get_size_and_eflags (conn) == -1)
> +          return -1;
> +        if (send_newstyle_option_reply_info_export (conn, option,
> +                                                    NBD_REP_INFO,
> +                                                    NBD_INFO_EXPORT) ==
-1)
> +          return -1;
> +
> +        /* For now we ignore all other info requests (but we must
> +         * ignore NBD_INFO_EXPORT if it was requested, because we
> +         * replied already above).  Therefore this loop doesn't do
> +         * much at the moment.
> +         */
> +        for (i = 0; i < nrinfos; ++i) {
> +          memcpy (&info, &data[4+exportnamelen+2+i*2], 2);
Spacing.
> +          info = be16toh (info);
> +          switch (info) {
> +          case NBD_INFO_EXPORT: /* ignore - reply sent above */ break;
> +          default:
> +            debug ("newstyle negotiation: NBD_OPT_GO: "
> +                   "ignoring NBD_INFO_* request %u", (unsigned)
info);
> +            break;
> +          }
> +        }
> +      }
> +
> +      /* Unlike NBD_OPT_EXPORT_NAME, NBD_OPT_GO sends back an ACK
> +       * or ERROR packet.
> +       */
> +      if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
> +        return -1;
> +
> +      break;
> +
>       default:
>         /* Unknown option. */
>         if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) ==
-1)
> @@ -712,10 +860,10 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>       }
>   
>       /* Note, since it's not very clear from the protocol doc, that
the
> -     * client must send NBD_OPT_EXPORT_NAME last, and that ends option
> -     * negotiation.
> +     * client must send NBD_OPT_EXPORT_NAME or NBD_OPT_GO last, and
> +     * that ends option negotiation.
>        */
> -    if (option == NBD_OPT_EXPORT_NAME)
> +    if (option == NBD_OPT_EXPORT_NAME || option == NBD_OPT_GO)
>         break;
NBD_OPT_EXPORT_NAME _always_ ends negotiation, since it has no error 
response. But NBD_OPT_GO only ends negotiation if you replied with an 
ACK. On the other hand, it looks like the code above only used 'break' 
when sending ACK, and used 'continue' for all error replies, so looks 
like you are good here.
>     }
>   
> @@ -733,6 +881,38 @@ _negotiate_handshake_newstyle_options (struct
connection *conn)
>       return -1;
>     }
>   
> +  /* If the last option was NBD_OPT_GO then we're all done. */
> +  if (option == NBD_OPT_GO)
> +    ;
> +  /* Else if the last option was NBD_OPT_EXPORT_NAME then we have to
> +   * finish the handshake by sending handshake_finish.
> +   */
> +  else if (option == NBD_OPT_EXPORT_NAME) {
> +    if (get_size_and_eflags (conn) == -1)
> +      return -1;
> +
> +    memset (&handshake_finish, 0, sizeof handshake_finish);
> +    handshake_finish.exportsize = htobe64 (conn->exportsize);
> +    handshake_finish.eflags = htobe16 (conn->eflags);
> +
> +    if (conn->send (conn,
> +                    &handshake_finish,
> +                    (conn->cflags & NBD_FLAG_NO_ZEROES)
> +                    ? offsetof (struct new_handshake_finish, zeroes)
> +                    : sizeof handshake_finish) == -1) {
> +      nbdkit_error ("write: %m");
> +      return -1;
> +    }
> +  }
> +  /* The client is buggy.  The last option must be NBD_OPT_GO or
> +   * NBD_OPT_EXPORT_NAME.
> +   */
> +  else {
This else clause is unreachable. You can only exit the loop by too many 
iterations (checked above), or by the post-switch break on just GO and 
EXPORT_NAME.
> +    nbdkit_error ("client options list didn't finish with
NBD_OPT_GO "
> +           "or NBD_OPT_EXPORT_NAME");
> +    return -1;
So instead of this unreachable error message, you could just abort().
> +  }
> +
>     return 0;
>   }
>   
> @@ -741,11 +921,6 @@ _negotiate_handshake_newstyle (struct connection
*conn)
>   {
>     struct new_handshake handshake;
>     uint16_t gflags;
> -  uint32_t cflags;
> -  struct new_handshake_finish handshake_finish;
> -  int64_t r;
> -  uint64_t exportsize;
> -  uint16_t eflags;
>   
>     gflags = NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES;
>   
> @@ -761,15 +936,15 @@ _negotiate_handshake_newstyle (struct connection
*conn)
>     }
>   
>     /* Client now sends us its 32 bit flags word ... */
> -  if (conn->recv (conn, &cflags, sizeof cflags) == -1) {
> +  if (conn->recv (conn, &conn->cflags, 4) == -1) {
The sizeof looks nicer than a magic number 4.

So, just style and dead code, no major issues with the logic changes.

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