[Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST

[Eric Blake]

Since we know that the maximum name we are willing to accept
is small enough to stack-allocate, rework the iteration over
NBD_OPT_LIST responses to reuse a stack buffer rather than
allocating every time.  Furthermore, we don't even have to
allocate if we know the server's length doesn't match what
we are searching for.

Not fixed here: Upstream NBD Protocol recently added this
clarification:
https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions

 Where this document refers to a string, then unless otherwise
 stated, that string is a sequence of UTF-8 code points, which
 is not NUL terminated, MUST NOT contain NUL characters, SHOULD
 be no longer than 256 bytes and MUST be no longer than 4096
 bytes. This applies to export names and error messages (amongst
 others).

To be fully compliant to that, we need to bump our export name
limit from 255 to at least 256, and need to decide whether we
can bump it higher (bumping it all the way to 4096 is annoying
in that we could no longer safely stack-allocate a worst-case
string, so we may still want to take the leeway offered by SHOULD
to force a reasonable smaller limit).

Signed-off-by: Eric Blake <address@hidden>
---
 nbd/client.c | 130 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 65 insertions(+), 65 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index b2dfc11..d4e37d5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -230,14 +230,17 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
nbd_opt_reply *reply,
     return result;
 }

-static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
+/* Return -1 if unrecoverable error occurs, 0 if NBD_OPT_LIST is
+ * unsupported, 1 if iteration is done, 2 to keep looking, and 3 if
+ * this entry matches want. */
+static int nbd_receive_list(QIOChannel *ioc, const char *want, Error **errp)
 {
     nbd_opt_reply reply;
     uint32_t len;
     uint32_t namelen;
+    char name[NBD_MAX_NAME_SIZE + 1];
     int error;

-    *name = NULL;
     if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
         return -1;
     }
@@ -252,97 +255,94 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, 
Error **errp)
             error_setg(errp, "length too long for option end");
             return -1;
         }
-    } else if (reply.type == NBD_REP_SERVER) {
-        if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
-            error_setg(errp, "incorrect option length %"PRIu32, len);
-            return -1;
-        }
-        if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
-            error_setg(errp, "failed to read option name length");
-            return -1;
-        }
-        namelen = be32_to_cpu(namelen);
-        len -= sizeof(namelen);
-        if (len < namelen) {
-            error_setg(errp, "incorrect option name length");
-            return -1;
-        }
-        if (namelen > NBD_MAX_NAME_SIZE) {
-            error_setg(errp, "export name length too long %" PRIu32, namelen);
-            return -1;
-        }
-
-        *name = g_new0(char, namelen + 1);
-        if (read_sync(ioc, *name, namelen) != namelen) {
-            error_setg(errp, "failed to read export name");
-            g_free(*name);
-            *name = NULL;
-            return -1;
-        }
-        (*name)[namelen] = '\0';
-        len -= namelen;
-        if (drop_sync(ioc, len) != len) {
-            error_setg(errp, "failed to read export description");
-            g_free(*name);
-            *name = NULL;
-            return -1;
-        }
-    } else {
+        return 1;
+    } else if (reply.type != NBD_REP_SERVER) {
         error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
                    reply.type, NBD_REP_SERVER);
         return -1;
     }
-    return 1;
+
+    if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
+        error_setg(errp, "incorrect option length %"PRIu32, len);
+        return -1;
+    }
+    if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+        error_setg(errp, "failed to read option name length");
+        return -1;
+    }
+    namelen = be32_to_cpu(namelen);
+    len -= sizeof(namelen);
+    if (len < namelen) {
+        error_setg(errp, "incorrect option name length");
+        return -1;
+    }
+    if (namelen != strlen(want)) {
+        if (drop_sync(ioc, len) != len) {
+            error_setg(errp, "failed to skip export name with wrong length");
+            return -1;
+        }
+        return 2;
+    }
+
+    assert(namelen < sizeof(name));
+    if (read_sync(ioc, name, namelen) != namelen) {
+        error_setg(errp, "failed to read export name");
+        return -1;
+    }
+    name[namelen] = '\0';
+    len -= namelen;
+    if (drop_sync(ioc, len) != len) {
+        error_setg(errp, "failed to read export description");
+        return -1;
+    }
+    return strcmp(name, want) == 0 ? 3 : 2;
 }


+/* Return -1 on failure, 0 if wantname is an available export. */
 static int nbd_receive_query_exports(QIOChannel *ioc,
                                      const char *wantname,
                                      Error **errp)
 {
     bool foundExport = false;

-    TRACE("Querying export list");
+    TRACE("Querying export list for '%s'", wantname);
     if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
         return -1;
     }

     TRACE("Reading available export names");
     while (1) {
-        char *name = NULL;
-        int ret = nbd_receive_list(ioc, &name, errp);
+        int ret = nbd_receive_list(ioc, wantname, errp);

-        if (ret < 0) {
-            g_free(name);
-            name = NULL;
+        switch (ret) {
+        default:
+            /* Server gave unexpected reply */
+            assert(ret < 0);
             return -1;
-        }
-        if (ret == 0) {
+        case 0:
             /* Server doesn't support export listing, so
              * we will just assume an export with our
              * wanted name exists */
-            foundExport = true;
-            break;
-        }
-        if (name == NULL) {
-            TRACE("End of export name list");
+            return 0;
+        case 1:
+            /* Done iterating. */
+            if (!foundExport) {
+                error_setg(errp, "No export with name '%s' available",
+                           wantname);
+                return -1;
+            }
+            return 0;
+        case 2:
+            /* Wasn't this one, keep going. */
             break;
-        }
-        if (g_str_equal(name, wantname)) {
+        case 3:
+            /* Found a match, but must finish parsing reply. */
+            TRACE("Found desired export name '%s'", wantname);
             foundExport = true;
-            TRACE("Found desired export name '%s'", name);
-        } else {
-            TRACE("Ignored export name '%s'", name);
+            break;
         }
-        g_free(name);
-    }
-
-    if (!foundExport) {
-        error_setg(errp, "No export with name '%s' available", wantname);
-        return -1;
     }
-
-    return 0;
 }

 static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
-- 
2.5.5