[Qemu-devel] [PATCH 14/18] nbd: Implement NBD_OPT_GO on client

[Eric Blake]

NBD_OPT_EXPORT_NAME is lousy: it doesn't have any sane error
reporting.  Upstream NBD recently added NBD_OPT_GO as the
improved version of the option that does what we want: it
reports sane errors on failures (including when a server
requires TLS but does not have NBD_OPT_GO!), and on success
it concludes with the same data as NBD_OPT_EXPORT_NAME sends.

Signed-off-by: Eric Blake <address@hidden>
---
 include/block/nbd.h |  1 +
 nbd/nbd-internal.h  |  7 +++++
 nbd/client.c        | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 35c0ea3..d261dbc 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -88,6 +88,7 @@ typedef struct nbd_reply nbd_reply;
 #define NBD_REP_ERR_POLICY      ((UINT32_C(1) << 31) | 2) /* Server denied */
 #define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. */
 #define NBD_REP_ERR_TLS_REQD    ((UINT32_C(1) << 31) | 5) /* TLS required */
+#define NBD_REP_ERR_UNKNOWN     ((UINT32_C(1) << 31) | 6) /* Export unknown */

 /* Request flags, sent from client to server during transmission phase */
 #define NBD_CMD_FLAG_FUA        (1 << 0)
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index b78d249..ddba1d0 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -55,8 +55,13 @@
  * https://github.com/yoe/nbd/blob/master/doc/proto.md
  */

+/* Size of all NBD_OPT_*, without payload */
 #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)
+/* Size of reply to NBD_OPT_EXPORT_NAME, without trailing zeroes */
+#define NBD_FINAL_REPLY_SIZE    (8 + 2)
+
 #define NBD_REQUEST_MAGIC       0x25609513
 #define NBD_REPLY_MAGIC         0x67446698
 #define NBD_OPTS_MAGIC          0x49484156454F5054LL
@@ -80,6 +85,8 @@
 #define NBD_OPT_LIST            (3)
 #define NBD_OPT_PEEK_EXPORT     (4)
 #define NBD_OPT_STARTTLS        (5)
+#define NBD_OPT_INFO            (6)
+#define NBD_OPT_GO              (7)

 /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
  * but only a limited set of errno values is specified in the protocol.
diff --git a/nbd/client.c b/nbd/client.c
index 507ddc1..af17d4c 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -215,6 +215,11 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
nbd_opt_reply *reply,
                    reply->option);
         break;

+    case NBD_REP_ERR_UNKNOWN:
+        error_setg(errp, "Requested export not available for option %" PRIx32,
+                   reply->option);
+        break;
+
     default:
         error_setg(errp, "Unknown error code when asking for option %" PRIx32,
                    reply->option);
@@ -299,6 +304,67 @@ static int nbd_receive_list(QIOChannel *ioc, const char 
*want, Error **errp)
 }


+/* Returns -1 if NBD_OPT_GO proves the export cannot be used, 0 if
+ * NBD_OPT_GO is unsupported (fall back to NBD_OPT_LIST and
+ * NBD_OPT_EXPORT_NAME in that case), and > 0 if the export is good to
+ * go (with the server data at the same point as it would be right
+ * after sending NBD_OPT_EXPORT_NAME). */
+static int nbd_opt_go(QIOChannel *ioc, const char *wantname, Error **errp)
+{
+    nbd_opt_reply reply;
+    uint32_t len;
+    uint32_t namelen;
+    int error;
+    char buf[NBD_MAX_NAME_SIZE];
+
+    TRACE("Attempting NBD_OPT_GO for export '%s'", wantname);
+    if (nbd_send_option_request(ioc, NBD_OPT_GO, -1, wantname, errp) < 0) {
+        return -1;
+    }
+
+    TRACE("Reading export info");
+    if (nbd_receive_option_reply(ioc, NBD_OPT_GO, &reply, errp) < 0) {
+        return -1;
+    }
+    error = nbd_handle_reply_err(ioc, &reply, errp);
+    if (error <= 0) {
+        return error;
+    }
+    len = reply.length;
+
+    if (reply.type != NBD_REP_SERVER) {
+        error_setg(errp, "unexpected reply type %" PRIx32 ", expected %x",
+                   reply.type, NBD_REP_SERVER);
+        return -1;
+    }
+
+    if (len < sizeof(namelen) + NBD_FINAL_REPLY_SIZE ||
+        len > sizeof(namelen) + sizeof(buf) + NBD_FINAL_REPLY_SIZE) {
+        error_setg(errp, "reply with %" PRIu32 " bytes is unexpected size",
+                   len);
+        return -1;
+    }
+
+    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+        error_setg(errp, "failed to read namelen");
+        return -1;
+    }
+    be32_to_cpus(&namelen);
+    if (len != sizeof(namelen) + namelen + NBD_FINAL_REPLY_SIZE) {
+        error_setg(errp, "namelen %" PRIu32 " is unexpected size",
+                   len);
+        return -1;
+    }
+
+    if (read_sync(ioc, buf, namelen) != namelen) {
+        error_setg(errp, "failed to read name");
+        return -1;
+    }
+
+    TRACE("export is good to go");
+    return 1;
+}
+
 /* Return -1 on failure, 0 if wantname is an available export. */
 static int nbd_receive_query_exports(QIOChannel *ioc,
                                      const char *wantname,
@@ -505,11 +571,26 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
             name = "";
         }
         if (fixedNewStyle) {
+            int result;
+
+            /* Try NBD_OPT_GO first - if it works, we are done (it
+             * also gives us a good message if the server requires
+             * TLS).  If it is not available, fall back to
+             * NBD_OPT_LIST for nicer error messages about a missing
+             * export, then use NBD_OPT_EXPORT_NAME.  */
+            result = nbd_opt_go(ioc, name, errp);
+            if (result < 0) {
+                goto fail;
+            }
+            if (result > 0) {
+                zeroes = false;
+                goto success;
+            }
             /* Check our desired export is present in the
              * server export list. Since NBD_OPT_EXPORT_NAME
              * cannot return an error message, running this
-             * query gives us good error reporting if the
-             * server required TLS
+             * query gives us better error reporting if the
+             * export name is not available.
              */
             if (nbd_receive_query_exports(ioc, name, errp) < 0) {
                 goto fail;
@@ -522,6 +603,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char 
*name, uint32_t *flags,
         }

         /* Read the response */
+    success:
         if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
             error_setg(errp, "Failed to read export length");
             goto fail;
-- 
2.5.5