[Qemu-devel] [PULL 0/6] NBD patches through 17 Jan

[Qemu-devel] [PULL 0/6] NBD patches through 17 Jan

[Eric Blake]

The following changes since commit 8e5dc9ba49743b46d955ec7dacb04e42ae7ada7c:

  Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180116' into staging (2018-01-16 17:36:39 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-01-17

for you to fetch changes up to 1d17922a28f5b9a8adcea3c93f49086e0a751e86:

  nbd/server: structurize option reply sending (2018-01-17 20:14:12 -0600)

I'm still waiting for v3 of Vladimir's "0/6 nbd export qmp interface",
but it's been a week since my last pull, so it's worth flushing the queue.

----------------------------------------------------------------
pull-nbd-2018-01-17

- Vladimir Sementsov-Ogievskiy/Eric Blake: 0/6 NBD server refactoring

----------------------------------------------------------------
Eric Blake (4):
      nbd/server: Hoist nbd_reject_length() earlier
      nbd/server: Better error for NBD_OPT_EXPORT_NAME failure
      nbd/server: Add va_list form of nbd_negotiate_send_rep_err()
      nbd/server: Add helper functions for parsing option payload

Vladimir Sementsov-Ogievskiy (2):
      nbd/server: refactor negotiation functions parameters
      nbd/server: structurize option reply sending

 nbd/server.c | 342 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 171 insertions(+), 171 deletions(-)

-- 
2.14.3

[Qemu-devel] [PULL 1/6] nbd/server: Hoist nbd_reject_length() earlier

[Eric Blake]

No semantic change, but will make it easier for an upcoming patch
to refactor code without having to add forward declarations.  Fix
a poor comment while at it.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180110230825.18321-2-eblake@redhat.com>
---
 nbd/server.c | 58 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 6cf2eeb2c1a..5ba12f3c0b3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1,5 +1,5 @@
 /*
- *  Copyright (C) 2016-2017 Red Hat, Inc.
+ *  Copyright (C) 2016-2018 Red Hat, Inc.
  *  Copyright (C) 2005  Anthony Liguori <anthony@codemonkey.ws>
  *
  *  Network Block Device Server Side
@@ -348,6 +348,34 @@ static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt,
     return 0;
 }

+/* nbd_reject_length: Handle any unexpected payload.
+ * @fatal requests that we quit talking to the client, even if we are able
+ * to successfully send an error reply.
+ * Return:
+ * -errno  transmission error occurred or @fatal was requested, errp is set
+ * 0       error message successfully sent to client, errp is not set
+ */
+static int nbd_reject_length(NBDClient *client, uint32_t length,
+                             uint32_t option, bool fatal, Error **errp)
+{
+    int ret;
+
+    assert(length);
+    if (nbd_drop(client->ioc, length, errp) < 0) {
+        return -EIO;
+    }
+    ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,
+                                     option, errp,
+                                     "option '%s' should have zero length",
+                                     nbd_opt_lookup(option));
+    if (fatal && !ret) {
+        error_setg(errp, "option '%s' should have zero length",
+                   nbd_opt_lookup(option));
+        return -EINVAL;
+    }
+    return ret;
+}
+
 /* Handle NBD_OPT_INFO and NBD_OPT_GO.
  * Return -errno on error, 0 if ready for next option, and 1 to move
  * into transmission phase.  */
@@ -570,34 +598,6 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     return QIO_CHANNEL(tioc);
 }

-/* nbd_reject_length: Handle any unexpected payload.
- * @fatal requests that we quit talking to the client, even if we are able
- * to successfully send an error to the guest.
- * Return:
- * -errno  transmission error occurred or @fatal was requested, errp is set
- * 0       error message successfully sent to client, errp is not set
- */
-static int nbd_reject_length(NBDClient *client, uint32_t length,
-                             uint32_t option, bool fatal, Error **errp)
-{
-    int ret;
-
-    assert(length);
-    if (nbd_drop(client->ioc, length, errp) < 0) {
-        return -EIO;
-    }
-    ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,
-                                     option, errp,
-                                     "option '%s' should have zero length",
-                                     nbd_opt_lookup(option));
-    if (fatal && !ret) {
-        error_setg(errp, "option '%s' should have zero length",
-                   nbd_opt_lookup(option));
-        return -EINVAL;
-    }
-    return ret;
-}
-
 /* nbd_negotiate_options
  * Process all NBD_OPT_* client option commands, during fixed newstyle
  * negotiation.
-- 
2.14.3

[Qemu-devel] [PULL 2/6] nbd/server: refactor negotiation functions parameters

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Instead of passing currently negotiating option and its length to
many of negotiation functions let's just store them on NBDClient
struct to be state-variables of negotiation phase.

This unifies semantics of negotiation functions and allows
tracking changes of remaining option length in future patches.

Asssert that optlen is back to 0 after negotiation (including
old-style connections which don't negotiate), although we need
more patches before we can assert optlen is 0 between options
during negotiation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171122101958.17065-2-vsementsov@virtuozzo.com>
[eblake: rebase, commit message tweak, assert !optlen after
negotiation completes]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 168 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 84 insertions(+), 84 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 5ba12f3c0b3..b9efdcc605d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -102,9 +102,11 @@ struct NBDClient {
     bool closing;

     bool structured_reply;
-};

-/* That's all folks */
+    uint32_t opt; /* Current option being negotiated */
+    uint32_t optlen; /* remaining length of data in ioc for the option being
+                        negotiated now */
+};

 static void nbd_client_receive_next_request(NBDClient *client);

@@ -137,10 +139,12 @@ static void nbd_client_receive_next_request(NBDClient *client);

 /* Send a reply header, including length, but no payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,
-                                      uint32_t opt, uint32_t len, Error **errp)
+static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
+                                      uint32_t len, Error **errp)
 {
     uint64_t magic;
+    QIOChannel *ioc = client->ioc;
+    uint32_t opt = client->opt;

     trace_nbd_negotiate_send_rep_len(opt, nbd_opt_lookup(opt),
                                      type, nbd_rep_lookup(type), len);
@@ -174,17 +178,17 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, uint32_t type,

 /* Send a reply header with default 0 length.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep(QIOChannel *ioc, uint32_t type, uint32_t opt,
+static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type,
                                   Error **errp)
 {
-    return nbd_negotiate_send_rep_len(ioc, type, opt, 0, errp);
+    return nbd_negotiate_send_rep_len(client, type, 0, errp);
 }

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int GCC_FMT_ATTR(5, 6)
-nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
-                           uint32_t opt, Error **errp, const char *fmt, ...)
+static int GCC_FMT_ATTR(4, 5)
+nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
+                           Error **errp, const char *fmt, ...)
 {
     va_list va;
     char *msg;
@@ -197,11 +201,11 @@ nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
     len = strlen(msg);
     assert(len < 4096);
     trace_nbd_negotiate_send_rep_err(msg);
-    ret = nbd_negotiate_send_rep_len(ioc, type, opt, len, errp);
+    ret = nbd_negotiate_send_rep_len(client, type, len, errp);
     if (ret < 0) {
         goto out;
     }
-    if (nbd_write(ioc, msg, len, errp) < 0) {
+    if (nbd_write(client->ioc, msg, len, errp) < 0) {
         error_prepend(errp, "write failed (error message): ");
         ret = -EIO;
     } else {
@@ -215,21 +219,21 @@ out:

 /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp,
+static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
                                        Error **errp)
 {
     size_t name_len, desc_len;
     uint32_t len;
     const char *name = exp->name ? exp->name : "";
     const char *desc = exp->description ? exp->description : "";
+    QIOChannel *ioc = client->ioc;
     int ret;

     trace_nbd_negotiate_send_rep_list(name, desc);
     name_len = strlen(name);
     desc_len = strlen(desc);
     len = name_len + desc_len + sizeof(len);
-    ret = nbd_negotiate_send_rep_len(ioc, NBD_REP_SERVER, NBD_OPT_LIST, len,
-                                     errp);
+    ret = nbd_negotiate_send_rep_len(client, NBD_REP_SERVER, len, errp);
     if (ret < 0) {
         return ret;
     }
@@ -258,20 +262,21 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp,
 static int nbd_negotiate_handle_list(NBDClient *client, Error **errp)
 {
     NBDExport *exp;
+    assert(client->opt == NBD_OPT_LIST);

     /* For each export, send a NBD_REP_SERVER reply. */
     QTAILQ_FOREACH(exp, &exports, next) {
-        if (nbd_negotiate_send_rep_list(client->ioc, exp, errp)) {
+        if (nbd_negotiate_send_rep_list(client, exp, errp)) {
             return -EINVAL;
         }
     }
     /* Finish with a NBD_REP_ACK. */
-    return nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, NBD_OPT_LIST, errp);
+    return nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
 }

 /* Send a reply to NBD_OPT_EXPORT_NAME.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
+static int nbd_negotiate_handle_export_name(NBDClient *client,
                                             uint16_t myflags, bool no_zeroes,
                                             Error **errp)
 {
@@ -288,15 +293,16 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
         [10 .. 133]   reserved     (0) [unless no_zeroes]
      */
     trace_nbd_negotiate_handle_export_name();
-    if (length >= sizeof(name)) {
+    if (client->optlen >= sizeof(name)) {
         error_setg(errp, "Bad length received");
         return -EINVAL;
     }
-    if (nbd_read(client->ioc, name, length, errp) < 0) {
+    if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {
         error_prepend(errp, "read failed: ");
         return -EINVAL;
     }
-    name[length] = '\0';
+    name[client->optlen] = '\0';
+    client->optlen = 0;

     trace_nbd_negotiate_handle_export_name_request(name);

@@ -326,14 +332,14 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
 /* Send a single NBD_REP_INFO, with a buffer @buf of @length bytes.
  * The buffer does NOT include the info type prefix.
  * Return -errno on error, 0 if ready to send more. */
-static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt,
+static int nbd_negotiate_send_info(NBDClient *client,
                                    uint16_t info, uint32_t length, void *buf,
                                    Error **errp)
 {
     int rc;

     trace_nbd_negotiate_send_info(info, nbd_info_lookup(info), length);
-    rc = nbd_negotiate_send_rep_len(client->ioc, NBD_REP_INFO, opt,
+    rc = nbd_negotiate_send_rep_len(client, NBD_REP_INFO,
                                     sizeof(info) + length, errp);
     if (rc < 0) {
         return rc;
@@ -355,22 +361,20 @@ static int nbd_negotiate_send_info(NBDClient *client, uint32_t opt,
  * -errno  transmission error occurred or @fatal was requested, errp is set
  * 0       error message successfully sent to client, errp is not set
  */
-static int nbd_reject_length(NBDClient *client, uint32_t length,
-                             uint32_t option, bool fatal, Error **errp)
+static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
 {
     int ret;

-    assert(length);
-    if (nbd_drop(client->ioc, length, errp) < 0) {
+    assert(client->optlen);
+    if (nbd_drop(client->ioc, client->optlen, errp) < 0) {
         return -EIO;
     }
-    ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,
-                                     option, errp,
+    ret = nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, errp,
                                      "option '%s' should have zero length",
-                                     nbd_opt_lookup(option));
+                                     nbd_opt_lookup(client->opt));
     if (fatal && !ret) {
         error_setg(errp, "option '%s' should have zero length",
-                   nbd_opt_lookup(option));
+                   nbd_opt_lookup(client->opt));
         return -EINVAL;
     }
     return ret;
@@ -379,8 +383,7 @@ static int nbd_reject_length(NBDClient *client, uint32_t length,
 /* Handle NBD_OPT_INFO and NBD_OPT_GO.
  * Return -errno on error, 0 if ready for next option, and 1 to move
  * into transmission phase.  */
-static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
-                                     uint32_t opt, uint16_t myflags,
+static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
                                      Error **errp)
 {
     int rc;
@@ -401,7 +404,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
         2 bytes: N, number of requests (can be 0)
         N * 2 bytes: N requests
     */
-    if (length < sizeof(namelen) + sizeof(requests)) {
+    if (client->optlen < sizeof(namelen) + sizeof(requests)) {
         msg = "overall request too short";
         goto invalid;
     }
@@ -409,8 +412,10 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
         return -EIO;
     }
     be32_to_cpus(&namelen);
-    length -= sizeof(namelen);
-    if (namelen > length - sizeof(requests) || (length - namelen) % 2) {
+    client->optlen -= sizeof(namelen);
+    if (namelen > client->optlen - sizeof(requests) ||
+        (client->optlen - namelen) % 2)
+    {
         msg = "name length is incorrect";
         goto invalid;
     }
@@ -422,16 +427,16 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
         return -EIO;
     }
     name[namelen] = '\0';
-    length -= namelen;
+    client->optlen -= namelen;
     trace_nbd_negotiate_handle_export_name_request(name);

     if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) {
         return -EIO;
     }
     be16_to_cpus(&requests);
-    length -= sizeof(requests);
+    client->optlen -= sizeof(requests);
     trace_nbd_negotiate_handle_info_requests(requests);
-    if (requests != length / sizeof(request)) {
+    if (requests != client->optlen / sizeof(request)) {
         msg = "incorrect number of  requests for overall length";
         goto invalid;
     }
@@ -440,7 +445,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
             return -EIO;
         }
         be16_to_cpus(&request);
-        length -= sizeof(request);
+        client->optlen -= sizeof(request);
         trace_nbd_negotiate_handle_info_request(request,
                                                 nbd_info_lookup(request));
         /* We care about NBD_INFO_NAME and NBD_INFO_BLOCK_SIZE;
@@ -455,18 +460,18 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
             break;
         }
     }
-    assert(length == 0);
+    assert(client->optlen == 0);

     exp = nbd_export_find(name);
     if (!exp) {
-        return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_UNKNOWN,
-                                          opt, errp, "export '%s' not present",
+        return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN,
+                                          errp, "export '%s' not present",
                                           name);
     }

     /* Don't bother sending NBD_INFO_NAME unless client requested it */
     if (sendname) {
-        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_NAME, namelen, name,
+        rc = nbd_negotiate_send_info(client, NBD_INFO_NAME, namelen, name,
                                      errp);
         if (rc < 0) {
             return rc;
@@ -478,7 +483,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
     if (exp->description) {
         size_t len = strlen(exp->description);

-        rc = nbd_negotiate_send_info(client, opt, NBD_INFO_DESCRIPTION,
+        rc = nbd_negotiate_send_info(client, NBD_INFO_DESCRIPTION,
                                      len, exp->description, errp);
         if (rc < 0) {
             return rc;
@@ -490,7 +495,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
      * whether this is OPT_INFO or OPT_GO. */
     /* minimum - 1 for back-compat, or 512 if client is new enough.
      * TODO: consult blk_bs(blk)->bl.request_alignment? */
-    sizes[0] = (opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
+    sizes[0] =
+            (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
     /* preferred - Hard-code to 4096 for now.
      * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
     sizes[1] = 4096;
@@ -500,7 +506,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
     cpu_to_be32s(&sizes[0]);
     cpu_to_be32s(&sizes[1]);
     cpu_to_be32s(&sizes[2]);
-    rc = nbd_negotiate_send_info(client, opt, NBD_INFO_BLOCK_SIZE,
+    rc = nbd_negotiate_send_info(client, NBD_INFO_BLOCK_SIZE,
                                  sizeof(sizes), sizes, errp);
     if (rc < 0) {
         return rc;
@@ -511,7 +517,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
                                              exp->nbdflags | myflags);
     stq_be_p(buf, exp->size);
     stw_be_p(buf + 8, exp->nbdflags | myflags);
-    rc = nbd_negotiate_send_info(client, opt, NBD_INFO_EXPORT,
+    rc = nbd_negotiate_send_info(client, NBD_INFO_EXPORT,
                                  sizeof(buf), buf, errp);
     if (rc < 0) {
         return rc;
@@ -521,21 +527,21 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
      * request block sizes, return an error.
      * TODO: consult blk_bs(blk)->request_align, and only error if it
      * is not 1? */
-    if (opt == NBD_OPT_INFO && !blocksize) {
-        return nbd_negotiate_send_rep_err(client->ioc,
-                                          NBD_REP_ERR_BLOCK_SIZE_REQD, opt,
+    if (client->opt == NBD_OPT_INFO && !blocksize) {
+        return nbd_negotiate_send_rep_err(client,
+                                          NBD_REP_ERR_BLOCK_SIZE_REQD,
                                           errp,
                                           "request NBD_INFO_BLOCK_SIZE to "
                                           "use this export");
     }

     /* Final reply */
-    rc = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, opt, errp);
+    rc = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
     if (rc < 0) {
         return rc;
     }

-    if (opt == NBD_OPT_GO) {
+    if (client->opt == NBD_OPT_GO) {
         client->exp = exp;
         QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
         nbd_export_get(client->exp);
@@ -544,10 +550,10 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
     return rc;

  invalid:
-    if (nbd_drop(client->ioc, length, errp) < 0) {
+    if (nbd_drop(client->ioc, client->optlen, errp) < 0) {
         return -EIO;
     }
-    return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID, opt,
+    return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID,
                                       errp, "%s", msg);
 }

@@ -561,11 +567,12 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     QIOChannelTLS *tioc;
     struct NBDTLSHandshakeData data = { 0 };

+    assert(client->opt == NBD_OPT_STARTTLS);
+
     trace_nbd_negotiate_handle_starttls();
     ioc = client->ioc;

-    if (nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
-                               NBD_OPT_STARTTLS, errp) < 0) {
+    if (nbd_negotiate_send_rep(client, NBD_REP_ACK, errp) < 0) {
         return NULL;
     }

@@ -670,12 +677,14 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
             return -EINVAL;
         }
         option = be32_to_cpu(option);
+        client->opt = option;

         if (nbd_read(client->ioc, &length, sizeof(length), errp) < 0) {
             error_prepend(errp, "read failed: ");
             return -EINVAL;
         }
         length = be32_to_cpu(length);
+        client->optlen = length;

         if (length > NBD_MAX_BUFFER_SIZE) {
             error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
@@ -697,8 +706,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 if (length) {
                     /* Unconditionally drop the connection if the client
                      * can't start a TLS negotiation correctly */
-                    return nbd_reject_length(client, length, option, true,
-                                             errp);
+                    return nbd_reject_length(client, true, errp);
                 }
                 tioc = nbd_negotiate_handle_starttls(client, errp);
                 if (!tioc) {
@@ -719,9 +727,8 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 if (nbd_drop(client->ioc, length, errp) < 0) {
                     return -EIO;
                 }
-                ret = nbd_negotiate_send_rep_err(client->ioc,
-                                                 NBD_REP_ERR_TLS_REQD,
-                                                 option, errp,
+                ret = nbd_negotiate_send_rep_err(client,
+                                                 NBD_REP_ERR_TLS_REQD, errp,
                                                  "Option 0x%" PRIx32
                                                  "not permitted before TLS",
                                                  option);
@@ -737,8 +744,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
             switch (option) {
             case NBD_OPT_LIST:
                 if (length) {
-                    ret = nbd_reject_length(client, length, option, false,
-                                            errp);
+                    ret = nbd_reject_length(client, false, errp);
                 } else {
                     ret = nbd_negotiate_handle_list(client, errp);
                 }
@@ -748,18 +754,17 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 /* NBD spec says we must try to reply before
                  * disconnecting, but that we must also tolerate
                  * guests that don't wait for our reply. */
-                nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK, option, NULL);
+                nbd_negotiate_send_rep(client, NBD_REP_ACK, NULL);
                 return 1;

             case NBD_OPT_EXPORT_NAME:
-                return nbd_negotiate_handle_export_name(client, length,
+                return nbd_negotiate_handle_export_name(client,
                                                         myflags, no_zeroes,
                                                         errp);

             case NBD_OPT_INFO:
             case NBD_OPT_GO:
-                ret = nbd_negotiate_handle_info(client, length, option,
-                                                myflags, errp);
+                ret = nbd_negotiate_handle_info(client, myflags, errp);
                 if (ret == 1) {
                     assert(option == NBD_OPT_GO);
                     return 0;
@@ -768,32 +773,27 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,

             case NBD_OPT_STARTTLS:
                 if (length) {
-                    ret = nbd_reject_length(client, length, option, false,
-                                            errp);
+                    ret = nbd_reject_length(client, false, errp);
                 } else if (client->tlscreds) {
-                    ret = nbd_negotiate_send_rep_err(client->ioc,
-                                                     NBD_REP_ERR_INVALID,
-                                                     option, errp,
+                    ret = nbd_negotiate_send_rep_err(client,
+                                                     NBD_REP_ERR_INVALID, errp,
                                                      "TLS already enabled");
                 } else {
-                    ret = nbd_negotiate_send_rep_err(client->ioc,
-                                                     NBD_REP_ERR_POLICY,
-                                                     option, errp,
+                    ret = nbd_negotiate_send_rep_err(client,
+                                                     NBD_REP_ERR_POLICY, errp,
                                                      "TLS not configured");
                 }
                 break;

             case NBD_OPT_STRUCTURED_REPLY:
                 if (length) {
-                    ret = nbd_reject_length(client, length, option, false,
-                                            errp);
+                    ret = nbd_reject_length(client, false, errp);
                 } else if (client->structured_reply) {
                     ret = nbd_negotiate_send_rep_err(
-                        client->ioc, NBD_REP_ERR_INVALID, option, errp,
+                        client, NBD_REP_ERR_INVALID, errp,
                         "structured reply already negotiated");
                 } else {
-                    ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
-                                                 option, errp);
+                    ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
                     client->structured_reply = true;
                     myflags |= NBD_FLAG_SEND_DF;
                 }
@@ -803,9 +803,8 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 if (nbd_drop(client->ioc, length, errp) < 0) {
                     return -EIO;
                 }
-                ret = nbd_negotiate_send_rep_err(client->ioc,
-                                                 NBD_REP_ERR_UNSUP,
-                                                 option, errp,
+                ret = nbd_negotiate_send_rep_err(client,
+                                                 NBD_REP_ERR_UNSUP, errp,
                                                  "Unsupported option 0x%"
                                                  PRIx32 " (%s)", option,
                                                  nbd_opt_lookup(option));
@@ -818,7 +817,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
              */
             switch (option) {
             case NBD_OPT_EXPORT_NAME:
-                return nbd_negotiate_handle_export_name(client, length,
+                return nbd_negotiate_handle_export_name(client,
                                                         myflags, no_zeroes,
                                                         errp);

@@ -898,6 +897,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
         }
     }

+    assert(!client->optlen);
     trace_nbd_negotiate_success();

     return 0;
-- 
2.14.3

[Qemu-devel] [PULL 3/6] nbd/server: Better error for NBD_OPT_EXPORT_NAME failure

[Eric Blake]

When a client abruptly disconnects before we've finished reading
the name sent with NBD_OPT_EXPORT_NAME, we are better off logging
the failure as EIO (we can't communicate with the client), rather
than EINVAL (the client sent bogus data).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180110230825.18321-4-eblake@redhat.com>
---
 nbd/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index b9efdcc605d..31c1d324297 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -299,7 +299,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client,
     }
     if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {
         error_prepend(errp, "read failed: ");
-        return -EINVAL;
+        return -EIO;
     }
     name[client->optlen] = '\0';
     client->optlen = 0;
-- 
2.14.3

[Qemu-devel] [PULL 4/6] nbd/server: Add va_list form of nbd_negotiate_send_rep_err()

[Eric Blake]

This will be useful for the next patch.

Based on a patch by Vladimir Sementsov-Ogievskiy

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180110230825.18321-5-eblake@redhat.com>
---
 nbd/server.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 31c1d324297..04da1c2f1e1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -186,18 +186,15 @@ static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type,

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int GCC_FMT_ATTR(4, 5)
-nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
-                           Error **errp, const char *fmt, ...)
+static int GCC_FMT_ATTR(4, 0)
+nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
+                            Error **errp, const char *fmt, va_list va)
 {
-    va_list va;
     char *msg;
     int ret;
     size_t len;

-    va_start(va, fmt);
     msg = g_strdup_vprintf(fmt, va);
-    va_end(va);
     len = strlen(msg);
     assert(len < 4096);
     trace_nbd_negotiate_send_rep_err(msg);
@@ -217,6 +214,21 @@ out:
     return ret;
 }

+/* Send an error reply.
+ * Return -errno on error, 0 on success. */
+static int GCC_FMT_ATTR(4, 5)
+nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
+                           Error **errp, const char *fmt, ...)
+{
+    va_list va;
+    int ret;
+
+    va_start(va, fmt);
+    ret = nbd_negotiate_send_rep_verr(client, type, errp, fmt, va);
+    va_end(va);
+    return ret;
+}
+
 /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
  * Return -errno on error, 0 on success. */
 static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
-- 
2.14.3

[Qemu-devel] [PULL 5/6] nbd/server: Add helper functions for parsing option payload

[Eric Blake]

Rather than making every callsite perform length sanity checks
and error reporting, add the helper functions nbd_opt_read()
and nbd_opt_drop() that use the length stored in the client
struct; also add an assertion that optlen is 0 before any
option (ie. any previous option was fully handled), complementing
the assertion added in an earlier patch that optlen is 0 after
all negotiation completes.

Note that the call in nbd_negotiate_handle_export_name() does
not use the new helper (in part because the server cannot
reply to NBD_OPT_EXPORT_NAME - it either succeeds or the
connection drops).

Based on patches by Vladimir Sementsov-Ogievskiy.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20180110230825.18321-6-eblake@redhat.com>
---
 nbd/server.c | 122 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 62 insertions(+), 60 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 04da1c2f1e1..78b08f58913 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -229,6 +229,40 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
     return ret;
 }

+/* Drop remainder of the current option, and send a reply with the
+ * given error type and message. Return -errno on read or write
+ * failure; or 0 if connection is still live. */
+static int GCC_FMT_ATTR(4, 5)
+nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
+             const char *fmt, ...)
+{
+    int ret = nbd_drop(client->ioc, client->optlen, errp);
+    va_list va;
+
+    client->optlen = 0;
+    if (!ret) {
+        va_start(va, fmt);
+        ret = nbd_negotiate_send_rep_verr(client, type, errp, fmt, va);
+        va_end(va);
+    }
+    return ret;
+}
+
+/* Read size bytes from the unparsed payload of the current option.
+ * Return -errno on I/O error, 0 if option was completely handled by
+ * sending a reply about inconsistent lengths, or 1 on success. */
+static int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
+                        Error **errp)
+{
+    if (size > client->optlen) {
+        return nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp,
+                            "Inconsistent lengths in option %s",
+                            nbd_opt_lookup(client->opt));
+    }
+    client->optlen -= size;
+    return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO : 1;
+}
+
 /* Send a single NBD_REP_SERVER reply to NBD_OPT_LIST, including payload.
  * Return -errno on error, 0 on success. */
 static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
@@ -378,14 +412,11 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp)
     int ret;

     assert(client->optlen);
-    if (nbd_drop(client->ioc, client->optlen, errp) < 0) {
-        return -EIO;
-    }
-    ret = nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID, errp,
-                                     "option '%s' should have zero length",
-                                     nbd_opt_lookup(client->opt));
+    ret = nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp,
+                       "option '%s' has unexpected length",
+                       nbd_opt_lookup(client->opt));
     if (fatal && !ret) {
-        error_setg(errp, "option '%s' should have zero length",
+        error_setg(errp, "option '%s' has unexpected length",
                    nbd_opt_lookup(client->opt));
         return -EINVAL;
     }
@@ -408,7 +439,6 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
     bool blocksize = false;
     uint32_t sizes[3];
     char buf[sizeof(uint64_t) + sizeof(uint16_t)];
-    const char *msg;

     /* Client sends:
         4 bytes: L, name length (can be 0)
@@ -416,48 +446,34 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
         2 bytes: N, number of requests (can be 0)
         N * 2 bytes: N requests
     */
-    if (client->optlen < sizeof(namelen) + sizeof(requests)) {
-        msg = "overall request too short";
-        goto invalid;
-    }
-    if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) {
-        return -EIO;
+    rc = nbd_opt_read(client, &namelen, sizeof(namelen), errp);
+    if (rc <= 0) {
+        return rc;
     }
     be32_to_cpus(&namelen);
-    client->optlen -= sizeof(namelen);
-    if (namelen > client->optlen - sizeof(requests) ||
-        (client->optlen - namelen) % 2)
-    {
-        msg = "name length is incorrect";
-        goto invalid;
-    }
     if (namelen >= sizeof(name)) {
-        msg = "name too long for qemu";
-        goto invalid;
+        return nbd_opt_drop(client, NBD_REP_ERR_INVALID, errp,
+                            "name too long for qemu");
     }
-    if (nbd_read(client->ioc, name, namelen, errp) < 0) {
-        return -EIO;
+    rc = nbd_opt_read(client, name, namelen, errp);
+    if (rc <= 0) {
+        return rc;
     }
     name[namelen] = '\0';
-    client->optlen -= namelen;
     trace_nbd_negotiate_handle_export_name_request(name);

-    if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) {
-        return -EIO;
+    rc = nbd_opt_read(client, &requests, sizeof(requests), errp);
+    if (rc <= 0) {
+        return rc;
     }
     be16_to_cpus(&requests);
-    client->optlen -= sizeof(requests);
     trace_nbd_negotiate_handle_info_requests(requests);
-    if (requests != client->optlen / sizeof(request)) {
-        msg = "incorrect number of  requests for overall length";
-        goto invalid;
-    }
     while (requests--) {
-        if (nbd_read(client->ioc, &request, sizeof(request), errp) < 0) {
-            return -EIO;
+        rc = nbd_opt_read(client, &request, sizeof(request), errp);
+        if (rc <= 0) {
+            return rc;
         }
         be16_to_cpus(&request);
-        client->optlen -= sizeof(request);
         trace_nbd_negotiate_handle_info_request(request,
                                                 nbd_info_lookup(request));
         /* We care about NBD_INFO_NAME and NBD_INFO_BLOCK_SIZE;
@@ -472,7 +488,9 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
             break;
         }
     }
-    assert(client->optlen == 0);
+    if (client->optlen) {
+        return nbd_reject_length(client, false, errp);
+    }

     exp = nbd_export_find(name);
     if (!exp) {
@@ -560,13 +578,6 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
         rc = 1;
     }
     return rc;
-
- invalid:
-    if (nbd_drop(client->ioc, client->optlen, errp) < 0) {
-        return -EIO;
-    }
-    return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID,
-                                      errp, "%s", msg);
 }


@@ -696,6 +707,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
             return -EINVAL;
         }
         length = be32_to_cpu(length);
+        assert(!client->optlen);
         client->optlen = length;

         if (length > NBD_MAX_BUFFER_SIZE) {
@@ -736,14 +748,9 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 return -EINVAL;

             default:
-                if (nbd_drop(client->ioc, length, errp) < 0) {
-                    return -EIO;
-                }
-                ret = nbd_negotiate_send_rep_err(client,
-                                                 NBD_REP_ERR_TLS_REQD, errp,
-                                                 "Option 0x%" PRIx32
-                                                 "not permitted before TLS",
-                                                 option);
+                ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
+                                   "Option 0x%" PRIx32
+                                   "not permitted before TLS", option);
                 /* Let the client keep trying, unless they asked to
                  * quit. In this mode, we've already sent an error, so
                  * we can't ack the abort.  */
@@ -812,14 +819,9 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 break;

             default:
-                if (nbd_drop(client->ioc, length, errp) < 0) {
-                    return -EIO;
-                }
-                ret = nbd_negotiate_send_rep_err(client,
-                                                 NBD_REP_ERR_UNSUP, errp,
-                                                 "Unsupported option 0x%"
-                                                 PRIx32 " (%s)", option,
-                                                 nbd_opt_lookup(option));
+                ret = nbd_opt_drop(client, NBD_REP_ERR_UNSUP, errp,
+                                   "Unsupported option 0x%" PRIx32 " (%s)",
+                                   option, nbd_opt_lookup(option));
                 break;
             }
         } else {
-- 
2.14.3

[Qemu-devel] [PULL 6/6] nbd/server: structurize option reply sending

[Eric Blake]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20171122101958.17065-6-vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 78b08f58913..6caa8d17be7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -137,43 +137,29 @@ static void nbd_client_receive_next_request(NBDClient *client);

 */

+static inline void set_be_option_rep(NBDOptionReply *rep, uint32_t option,
+                                     uint32_t type, uint32_t length)
+{
+    stq_be_p(&rep->magic, NBD_REP_MAGIC);
+    stl_be_p(&rep->option, option);
+    stl_be_p(&rep->type, type);
+    stl_be_p(&rep->length, length);
+}
+
 /* Send a reply header, including length, but no payload.
  * Return -errno on error, 0 on success. */
 static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
                                       uint32_t len, Error **errp)
 {
-    uint64_t magic;
-    QIOChannel *ioc = client->ioc;
-    uint32_t opt = client->opt;
+    NBDOptionReply rep;

-    trace_nbd_negotiate_send_rep_len(opt, nbd_opt_lookup(opt),
+    trace_nbd_negotiate_send_rep_len(client->opt, nbd_opt_lookup(client->opt),
                                      type, nbd_rep_lookup(type), len);

     assert(len < NBD_MAX_BUFFER_SIZE);
-    magic = cpu_to_be64(NBD_REP_MAGIC);
-    if (nbd_write(ioc, &magic, sizeof(magic), errp) < 0) {
-        error_prepend(errp, "write failed (rep magic): ");
-        return -EINVAL;
-    }

-    opt = cpu_to_be32(opt);
-    if (nbd_write(ioc, &opt, sizeof(opt), errp) < 0) {
-        error_prepend(errp, "write failed (rep opt): ");
-        return -EINVAL;
-    }
-
-    type = cpu_to_be32(type);
-    if (nbd_write(ioc, &type, sizeof(type), errp) < 0) {
-        error_prepend(errp, "write failed (rep type): ");
-        return -EINVAL;
-    }
-
-    len = cpu_to_be32(len);
-    if (nbd_write(ioc, &len, sizeof(len), errp) < 0) {
-        error_prepend(errp, "write failed (rep data length): ");
-        return -EINVAL;
-    }
-    return 0;
+    set_be_option_rep(&rep, client->opt, type, len);
+    return nbd_write(client->ioc, &rep, sizeof(rep), errp);
 }

 /* Send a reply header with default 0 length.
-- 
2.14.3

Re: [Qemu-devel] [PULL 0/6] NBD patches through 17 Jan

[Peter Maydell]

On 18 January 2018 at 02:26, Eric Blake <eblake@redhat.com> wrote:
> The following changes since commit 8e5dc9ba49743b46d955ec7dacb04e42ae7ada7c:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20180116' into staging (2018-01-16 17:36:39 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/ericb.git tags/pull-nbd-2018-01-17
>
> for you to fetch changes up to 1d17922a28f5b9a8adcea3c93f49086e0a751e86:
>
>   nbd/server: structurize option reply sending (2018-01-17 20:14:12 -0600)
>
> I'm still waiting for v3 of Vladimir's "0/6 nbd export qmp interface",
> but it's been a week since my last pull, so it's worth flushing the queue.
>
> ----------------------------------------------------------------
> pull-nbd-2018-01-17
>
> - Vladimir Sementsov-Ogievskiy/Eric Blake: 0/6 NBD server refactoring
>
> ----------------------------------------------------------------
> Eric Blake (4):
>       nbd/server: Hoist nbd_reject_length() earlier
>       nbd/server: Better error for NBD_OPT_EXPORT_NAME failure
>       nbd/server: Add va_list form of nbd_negotiate_send_rep_err()
>       nbd/server: Add helper functions for parsing option payload
>
> Vladimir Sementsov-Ogievskiy (2):
>       nbd/server: refactor negotiation functions parameters
>       nbd/server: structurize option reply sending

Applied, thanks.

-- PMM