From ff100c505e2a1cdc0a405008513948caa935b6cb Mon Sep 17 00:00:00 2001

From: John Snow <jsnow@redhat.com>

Date: Wed, 27 Mar 2019 17:22:42 +0100

Subject: [PATCH 104/163] nbd/client: Refactor nbd_receive_list()

RH-Author: John Snow <jsnow@redhat.com>

Message-id: <20190327172308.31077-30-jsnow@redhat.com>

Patchwork-id: 85210

O-Subject: [RHEL-7.7 qemu-kvm-rhev PATCH 29/55] nbd/client: Refactor nbd_receive_list()

Bugzilla: 1691009

RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

RH-Acked-by: Max Reitz <mreitz@redhat.com>

RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>

From: Eric Blake <eblake@redhat.com>

Right now, nbd_receive_list() is only called by

nbd_receive_query_exports(), which in turn is only called if the

server lacks NBD_OPT_GO but has working option negotiation, and is

merely used as a quality-of-implementation trick since servers

can't give decent errors for NBD_OPT_EXPORT_NAME.  However, servers

that lack NBD_OPT_GO are becoming increasingly rare (nbdkit was a

latecomer, in Aug 2018, but qemu has been such a server since commit

f37708f6 in July 2017 and released in 2.10), so it no longer makes

sense to micro-optimize that function for performance.

Furthermore, when debugging a server's implementation, tracing the

full reply (both names and descriptions) is useful, not to mention

that upcoming patches adding 'qemu-nbd --list' will want to collect

that data.  And when you consider that a server can send an export

name up to the NBD protocol length limit of 4k; but our current

NBD_MAX_NAME_SIZE is only 256, we can't trace all valid server

names without more storage, but 4k is large enough that the heap

is better than the stack for long names.

Thus, I'm changing the division of labor, with nbd_receive_list()

now always malloc'ing a result on success (the malloc is bounded

by the fact that we reject servers with a reply length larger

than 32M), and moving the comparison to 'wantname' to the caller.

There is a minor change in behavior where a server with 0 exports

(an immediate NBD_REP_ACK reply) is now no longer distinguished

from a server without LIST support (NBD_REP_ERR_UNSUP); this

information could be preserved with a complication to the calling

contract to provide a bit more information, but I didn't see the

point.  After all, the worst that can happen if our guess at a

match is wrong is that the caller will get a cryptic disconnect

when NBD_OPT_EXPORT_NAME fails (which is no different from what

would happen if we had not tried LIST), while treating an empty

list as immediate failure would prevent connecting to really old

servers that really did lack LIST.  Besides, NBD servers with 0

exports are rare (qemu can do it when using QMP nbd-server-start

without nbd-server-add - but qemu understands NBD_OPT_GO and

thus won't tickle this change in behavior).

Fix the spelling of foundExport to match coding standards while

in the area.

Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

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

Message-Id: <20190117193658.16413-9-eblake@redhat.com>

(cherry picked from commit 091d0bf3c94737fc451a9b3f4eddf3a4d74c90b8)

Signed-off-by: John Snow <jsnow@redhat.com>

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

---

 nbd/client.c     | 91 ++++++++++++++++++++++++++++++++++++--------------------

 nbd/trace-events |  1 +

 2 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c

index f625c20..fd4ba8d 100644

--- a/nbd/client.c

+++ b/nbd/client.c

@@ -234,18 +234,24 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,

     return result;

 }

-/* Process another portion of the NBD_OPT_LIST reply.  Set *@match if

- * the current reply matches @want or if the server does not support

- * NBD_OPT_LIST, otherwise leave @match alone.  Return 0 if iteration

- * is complete, positive if more replies are expected, or negative

- * with @errp set if an unrecoverable error occurred. */

-static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,

+/* nbd_receive_list:

+ * Process another portion of the NBD_OPT_LIST reply, populating any

+ * name received into *@name. If @description is non-NULL, and the

+ * server provided a description, that is also populated. The caller

+ * must eventually call g_free() on success.

+ * Returns 1 if name and description were set and iteration must continue,

+ *         0 if iteration is complete (including if OPT_LIST unsupported),

+ *         -1 with @errp set if an unrecoverable error occurred.

+ */

+static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,

                             Error **errp)

 {

+    int ret = -1;

     NBDOptionReply reply;

     uint32_t len;

     uint32_t namelen;

-    char name[NBD_MAX_NAME_SIZE + 1];

+    char *local_name = NULL;

+    char *local_desc = NULL;

     int error;

     if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {

@@ -253,9 +259,6 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,

     }

     error = nbd_handle_reply_err(ioc, &reply, errp);

     if (error <= 0) {

-        /* The server did not support NBD_OPT_LIST, so set *match on

-         * the assumption that any name will be accepted.  */

-        *match = true;

         return error;

     }

     len = reply.length;

@@ -292,33 +295,38 @@ static int nbd_receive_list(QIOChannel *ioc, const char *want, bool *match,

         nbd_send_opt_abort(ioc);

         return -1;

     }

-    if (namelen != strlen(want)) {

-        if (nbd_drop(ioc, len, errp) < 0) {

-            error_prepend(errp,

-                          "failed to skip export name with wrong length: ");

-            nbd_send_opt_abort(ioc);

-            return -1;

-        }

-        return 1;

-    }

-    assert(namelen < sizeof(name));

-    if (nbd_read(ioc, name, namelen, errp) < 0) {

+    local_name = g_malloc(namelen + 1);

+    if (nbd_read(ioc, local_name, namelen, errp) < 0) {

         error_prepend(errp, "failed to read export name: ");

         nbd_send_opt_abort(ioc);

-        return -1;

+        goto out;

     }

-    name[namelen] = '\0';

+    local_name[namelen] = '\0';

     len -= namelen;

-    if (nbd_drop(ioc, len, errp) < 0) {

-        error_prepend(errp, "failed to read export description: ");

-        nbd_send_opt_abort(ioc);

-        return -1;

+    if (len) {

+        local_desc = g_malloc(len + 1);

+        if (nbd_read(ioc, local_desc, len, errp) < 0) {

+            error_prepend(errp, "failed to read export description: ");

+            nbd_send_opt_abort(ioc);

+            goto out;

+        }

+        local_desc[len] = '\0';

     }

-    if (!strcmp(name, want)) {

-        *match = true;

+

+    trace_nbd_receive_list(local_name, local_desc ?: "");

+    *name = local_name;

+    local_name = NULL;

+    if (description) {

+        *description = local_desc;

+        local_desc = NULL;

     }

-    return 1;

+    ret = 1;

+

+ out:

+    g_free(local_name);

+    g_free(local_desc);

+    return ret;

 }

@@ -493,7 +501,8 @@ static int nbd_receive_query_exports(QIOChannel *ioc,

                                      const char *wantname,

                                      Error **errp)

 {

-    bool foundExport = false;

+    bool list_empty = true;

+    bool found_export = false;

     trace_nbd_receive_query_exports_start(wantname);

     if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {

@@ -501,14 +510,25 @@ static int nbd_receive_query_exports(QIOChannel *ioc,

     }

     while (1) {

-        int ret = nbd_receive_list(ioc, wantname, &foundExport, errp);

+        char *name;

+        int ret = nbd_receive_list(ioc, &name, NULL, errp);

         if (ret < 0) {

             /* Server gave unexpected reply */

             return -1;

         } else if (ret == 0) {

             /* Done iterating. */

-            if (!foundExport) {

+            if (list_empty) {

+                /*

+                 * We don't have enough context to tell a server that

+                 * sent an empty list apart from a server that does

+                 * not support the list command; but as this function

+                 * is just used to trigger a nicer error message

+                 * before trying NBD_OPT_EXPORT_NAME, assume the

+                 * export is available.

+                 */

+                return 0;

+            } else if (!found_export) {

                 error_setg(errp, "No export with name '%s' available",

                            wantname);

                 nbd_send_opt_abort(ioc);

@@ -517,6 +537,11 @@ static int nbd_receive_query_exports(QIOChannel *ioc,

             trace_nbd_receive_query_exports_success(wantname);

             return 0;

         }

+        list_empty = false;

+        if (!strcmp(name, wantname)) {

+            found_export = true;

+        }

+        g_free(name);

     }

 }

diff --git a/nbd/trace-events b/nbd/trace-events

index 5492042..d1e1ca6 100644

--- a/nbd/trace-events

+++ b/nbd/trace-events

@@ -3,6 +3,7 @@ nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending o

 nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, uint32_t length) "Received option reply %" PRIu32" (%s), type %" PRIu32" (%s), len %" PRIu32

 nbd_server_error_msg(uint32_t err, const char *type, const char *msg) "server reported error 0x%" PRIx32 " (%s) with additional message: %s"

 nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request %" PRIu32 " (%s), attempting fallback"

+nbd_receive_list(const char *name, const char *desc) "export list includes '%s', description '%s'"

 nbd_opt_go_start(const char *name) "Attempting NBD_OPT_GO for export '%s'"

 nbd_opt_go_success(void) "Export is good to go"

 nbd_opt_go_info_unknown(int info, const char *name) "Ignoring unknown info %d (%s)"

-- 

1.8.3.1