[Qemu-devel] [PATCH v2 0/6] NBD server refactoring before BLOCK_STATUS

[Qemu-devel] [PATCH v2 0/6] NBD server refactoring before BLOCK_STATUS

[Eric Blake]

This is my promised revision of Vladimir's v1 posted here:
https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04119.html

Sorry for my delay; it was due in part to an embargo while dealing
with 2 bounds-check CVEs in the NBD code that I discovered while
reviewing his v1 (fixed in time for 2.11), then waiting for the
2.12 tree to reopen, coupled with my holiday break.  I'm hoping
we can get actual BLOCK_STATUS code reviewed and applied much
faster than this preliminary series has gone.

Based-on: <20180110225944.17920-1-eblake@redhat.com>

Since v1:
- original patch 4/5 now in a pull request
- replace original 2-3/5 with a single patch, giving more useful
semantics to nbd_opt_drop/nbd_opt_read
- add a couple of other easy fixes while touching the file

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 | 341 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 171 insertions(+), 170 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 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.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 56 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 6cf2eeb2c1..d3b457c337 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -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 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;
+}
+
 /* 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

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

[Vladimir Sementsov-Ogievskiy]

11.01.2018 02:08, Eric Blake wrote:
> No semantic change, but will make it easier for an upcoming patch
> to refactor code without having to add forward declarations.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

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

-- 

Best regards,
Vladimir

[Qemu-devel] [PATCH v2 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.

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 d3b457c337..08a24b56f4 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);

@@ -1707,6 +1706,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
         return;
     }

+    assert(!client->optlen);
     nbd_client_receive_next_request(client);
 }

-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: refactor negotiation functions parameters

[Vladimir Sementsov-Ogievskiy]

11.01.2018 02:08, Eric Blake wrote:
> 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.
>
> 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(-)

[..]

> @@ -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);
>
> @@ -1707,6 +1706,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
>           return;
>       }
>
> +    assert(!client->optlen);


hmm, should not it be at the end of nbd_negotiate() ? Looks OK anyway.

>       nbd_client_receive_next_request(client);
>   }
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: refactor negotiation functions parameters

[Eric Blake]

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

On 01/11/2018 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2018 02:08, Eric Blake wrote:
>> 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.
>>
>> 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(-)
> 

>> @@ -1707,6 +1706,7 @@ static coroutine_fn void
>> nbd_co_client_start(void *opaque)
>>           return;
>>       }
>>
>> +    assert(!client->optlen);
> 
> 
> hmm, should not it be at the end of nbd_negotiate() ? Looks OK anyway.

Sure, I'll squash this in, for no real change in behavior:

diff --git i/nbd/server.c w/nbd/server.c
index c22d5e6ecf..3fd145592e 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -902,6 +902,7 @@ static coroutine_fn int nbd_negotiate(NBDClient
*client, Error **errp)
         }
     }

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

     return 0;
@@ -1726,7 +1727,6 @@ static coroutine_fn void nbd_co_client_start(void
*opaque)
         return;
     }

-    assert(!client->optlen);
     nbd_client_receive_next_request(client);
 }



-- 
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 --]

Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: refactor negotiation functions parameters

[Vladimir Sementsov-Ogievskiy]

11.01.2018 22:54, Eric Blake wrote:
> On 01/11/2018 11:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 11.01.2018 02:08, Eric Blake wrote:
>>> 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.
>>>
>>> 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(-)
>>> @@ -1707,6 +1706,7 @@ static coroutine_fn void
>>> nbd_co_client_start(void *opaque)
>>>            return;
>>>        }
>>>
>>> +    assert(!client->optlen);
>>
>> hmm, should not it be at the end of nbd_negotiate() ? Looks OK anyway.
> Sure, I'll squash this in, for no real change in behavior:
>
> diff --git i/nbd/server.c w/nbd/server.c
> index c22d5e6ecf..3fd145592e 100644
> --- i/nbd/server.c
> +++ w/nbd/server.c
> @@ -902,6 +902,7 @@ static coroutine_fn int nbd_negotiate(NBDClient
> *client, Error **errp)
>           }
>       }
>
> +    assert(!client->optlen);
>       trace_nbd_negotiate_success();
>
>       return 0;
> @@ -1726,7 +1727,6 @@ static coroutine_fn void nbd_co_client_start(void
> *opaque)
>           return;
>       }
>
> -    assert(!client->optlen);
>       nbd_client_receive_next_request(client);
>   }
>
>
>


or, even better, in nbd_negotiate_options. and you actually do it in 5/6.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/6] nbd/server: refactor negotiation functions parameters

[Eric Blake]

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

On 01/12/2018 04:18 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> hmm, should not it be at the end of nbd_negotiate() ? Looks OK anyway.
>> Sure, I'll squash this in, for no real change in behavior:
>>
>> diff --git i/nbd/server.c w/nbd/server.c
>> index c22d5e6ecf..3fd145592e 100644
>> --- i/nbd/server.c
>> +++ w/nbd/server.c
>> @@ -902,6 +902,7 @@ static coroutine_fn int nbd_negotiate(NBDClient
>> *client, Error **errp)
>>           }
>>       }
>>
>> +    assert(!client->optlen);
>>       trace_nbd_negotiate_success();
>>

This says that after all negotiation is complete, optlen is 0 (so in
reality, it is only checking NBD_OPT_GO and NBD_OPT_EXPORT_NAME - since
those are the only two options that can end negotiation).  But it does
not check state in between individual options during the actual
negotiation.  Also, this is the only spot in the code that can check
that optlen is 0 even for old-style connections (where we do not call
nbd_negotiate_options), so it's a nice spot to prove that when we move
into transmission phase, we aren't stranding any unread data from
handshake phase.

> 
> 
> or, even better, in nbd_negotiate_options. and you actually do it in 5/6.

Whereas that is stating that optlen is 0 after every option that does
not return early (not possible in this patch, because we need the
nbd_opt_read() helper in 5/6 that clears optlen as we go) - and
meanwhile does NOT cover the places that return early (NBD_OPT_GO and
NBD_OPT_EXPORT_NAME, which we caught here).  So we need both assertions
at the end of the series, and I'm comfortable with introducing them in
separate patches.

-- 
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 --]

[Qemu-devel] [PATCH v2 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>
---
 nbd/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 08a24b56f4..9943a911c3 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

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

[Vladimir Sementsov-Ogievskiy]

11.01.2018 02:08, Eric Blake wrote:
> 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>

> ---
>   nbd/server.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index 08a24b56f4..9943a911c3 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;


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v2 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>
---
 nbd/server.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 9943a911c3..d23bc2918a 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

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

[Vladimir Sementsov-Ogievskiy]

11.01.2018 02:08, Eric Blake wrote:
> This will be useful for the next patch.
>
> Based on a patch by Vladimir Sementsov-Ogievskiy
>
> Signed-off-by: Eric Blake <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 9943a911c3..d23bc2918a 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)

Hmm you placed fmt and va after errp. Previously we discussed one 
exclusion from "errp should be last" -
"...", variable number of arguments. So, it is new exclusion (or I 
missed something?).. Looks good for me,
anyway, as it corresponds to "errp, fmt, ..." notation.

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

>   {
> -    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,


-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 01/11/2018 12:05 PM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2018 02:08, Eric Blake wrote:
>> This will be useful for the next patch.
>>
>> Based on a patch by Vladimir Sementsov-Ogievskiy
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>   nbd/server.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>

>> -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)
> 
> Hmm you placed fmt and va after errp. Previously we discussed one
> exclusion from "errp should be last" -
> "...", variable number of arguments. So, it is new exclusion (or I
> missed something?).. Looks good for me,
> anyway, as it corresponds to "errp, fmt, ..." notation.

Indeed, it is precisely because I value consistency between 'fmt, ...'
and 'fmt, va_list' higher than 'errp last' that I rearranged things from
your patch into the form I used.  Or worded another way, even though
va_list is only one argument in name, I treat it the same as the
variable number of arguments that warrants the exception to the errp
last rule of thumb.

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

Thanks; I think this series is now ready for staging on my NBD queue;
although my next pull request won't be until after the weekend.

-- 
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 --]

[Qemu-devel] [PATCH v2 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 reduced to zero
after each option is handled.

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>
---
 nbd/server.c | 123 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 63 insertions(+), 60 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index d23bc2918a..ec8c3be019 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -229,6 +229,41 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
     return ret;
 }

+/* Drop remainder of the current option, after sending 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);
+
+    client->optlen = 0;
+    if (!ret) {
+        va_list va;
+
+        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 +413,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 +440,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 +447,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 +489,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 +579,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);
 }


@@ -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 {
@@ -842,6 +844,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
         if (ret < 0) {
             return ret;
         }
+        assert(!client->optlen);
     }
 }

-- 
2.14.3

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

[Vladimir Sementsov-Ogievskiy]

11.01.2018 02:08, Eric Blake wrote:
> 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 reduced to zero
> after each option is handled.
>
> 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>
> ---
>   nbd/server.c | 123 ++++++++++++++++++++++++++++++-----------------------------
>   1 file changed, 63 insertions(+), 60 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index d23bc2918a..ec8c3be019 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -229,6 +229,41 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
>       return ret;
>   }
>
> +/* Drop remainder of the current option, after sending a reply with

looks a bit weird: actually you drop the remainder _before_ sending a reply)

> + * the given error type and message. Return -errno on read or write

also, unrelated note, -errno is always forced to -EIO, because of 
nbd_read realization.
and this note applies to many other places here. It is correct (EIO is 
errno, why not?),
but it may be not bad to note it somewhere..

> + * 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);
> +
> +    client->optlen = 0;
> +    if (!ret) {
> +        va_list va;
> +
> +        va_start(va, fmt);
> +        ret = nbd_negotiate_send_rep_verr(client, type, errp, fmt, va);
> +        va_end(va);
> +    }
> +    return ret;
> +}

[..]

> @@ -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 {
> @@ -842,6 +844,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>           if (ret < 0) {
>               return ret;
>           }
> +        assert(!client->optlen);

isn't it from 2/6?

>       }
>   }
>

anyway,

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

-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 01/12/2018 04:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2018 02:08, Eric Blake wrote:
>> 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 reduced to zero
>> after each option is handled.
>>
>> 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).
>>

>>
>> +/* Drop remainder of the current option, after sending a reply with
> 
> looks a bit weird: actually you drop the remainder _before_ sending a
> reply)

Good catch. I'll fix it with s/after/and/

> 
>> + * the given error type and message. Return -errno on read or write
> 
> also, unrelated note, -errno is always forced to -EIO, because of
> nbd_read realization.
> and this note applies to many other places here. It is correct (EIO is
> errno, why not?),
> but it may be not bad to note it somewhere..

Someday nbd_read() might fail with something other than EIO (ESHUTDOWN,
perhaps?), in which case leaving this documented as -errno would be
better than hardcoding that EIO is the only failure for now.

>> @@ -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 {
>> @@ -842,6 +844,7 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>>           if (ret < 0) {
>>               return ret;
>>           }
>> +        assert(!client->optlen);
> 
> isn't it from 2/6?

No, this is a second instance of the assertion, the one that applies
between each option (which I couldn't do in 2/6 because not all options
were manipulating optlen back then).  Maybe I can tweak the commit
messages to make that more obvious.

> 
>>       }
>>   }
>>
> 
> anyway,
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks for the careful attention to detail.


-- 
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 --]

[Qemu-devel] [PATCH v2 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 ec8c3be019..9019ad28f4 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