Libguestfs - Jul 2020 - [PATCH libnbd PROPOSAL] Add APIs for listing exports from an NBD server.

Proposal for new APIs to list exports.  The general shape of the API
can probably best be seen from the examples/list-exports.c example.

Rich.

A major missing feature of this library was the ability to list
exports from an NBD server.  This implements the feature by adding a
new handle mode and additional functions for querying the list of
export names.
---
 .gitignore                               |   1 +
 examples/Makefile.am                     |  14 +++
 examples/list-exports.c                  | 108 +++++++++++++++++++
 generator/API.ml                         |  72 +++++++++++++
 generator/Makefile.am                    |   1 +
 generator/state_machine.ml               |  39 +++++++
 generator/states-newstyle-opt-list.c     | 131 +++++++++++++++++++++++
 generator/states-newstyle-opt-starttls.c |   8 +-
 lib/handle.c                             |  50 +++++++++
 lib/internal.h                           |   9 ++
 lib/nbd-protocol.h                       |   6 ++
 11 files changed, 435 insertions(+), 4 deletions(-)

diff --git a/.gitignore b/.gitignore
index 131dabc..3afbb78 100644
--- a/.gitignore
+++ b/.gitignore
@@ -59,6 +59,7 @@ Makefile.in
 /examples/fetch-first-sector
 /examples/get-size
 /examples/glib-main-loop
+/examples/list-exports
 /examples/open-qcow2
 /examples/reads-and-writes
 /examples/server-flags
diff --git a/examples/Makefile.am b/examples/Makefile.am
index aa63179..b99cac1 100644
--- a/examples/Makefile.am
+++ b/examples/Makefile.am
@@ -26,6 +26,7 @@ noinst_PROGRAMS = \
 	encryption \
 	fetch-first-sector \
 	get-size \
+	list-exports \
 	open-qcow2 \
 	reads-and-writes \
 	server-flags \
@@ -103,6 +104,19 @@ get_size_LDADD = \
 	$(top_builddir)/lib/libnbd.la \
 	$(NULL)
 
+list_exports_SOURCES = \
+	list-exports.c \
+	$(NULL)
+list_exports_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	$(NULL)
+list_exports_CFLAGS = \
+	$(WARNINGS_CFLAGS) \
+	$(NULL)
+list_exports_LDADD = \
+	$(top_builddir)/lib/libnbd.la \
+	$(NULL)
+
 open_qcow2_SOURCES = \
 	open-qcow2.c \
 	$(NULL)
diff --git a/examples/list-exports.c b/examples/list-exports.c
new file mode 100644
index 0000000..18035d4
--- /dev/null
+++ b/examples/list-exports.c
@@ -0,0 +1,108 @@
+/* This example shows how to list NBD exports.
+ *
+ * To test this with qemu-nbd:
+ *   $ qemu-nbd -x "hello" -t -k /tmp/sock disk.img
+ *   $ ./run examples/list-exports /tmp/sock
+ *   [0] hello
+ *   Which export to connect to? 0
+ *   Connecting to hello ...
+ *   /tmp/sock: hello: size = 2048 bytes
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <errno.h>
+
+#include <libnbd.h>
+
+int
+main (int argc, char *argv[])
+{
+  struct nbd_handle *nbd, *nbd2;
+  int r, i;
+  char *name;
+  int64_t size;
+
+  if (argc != 2) {
+    fprintf (stderr, "%s socket\n", argv[0]);
+    exit (EXIT_FAILURE);
+  }
+
+  /* Create the libnbd handle for querying exports. */
+  nbd = nbd_create ();
+  if (nbd == NULL) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Set the list exports mode in the handle. */
+  nbd_set_list_exports (nbd, 1);
+
+  /* Connect to the NBD server over a
+   * Unix domain socket.  A side effect of
+   * connecting is to list the exports.
+   * This operation can fail normally, so
+   * we need to check the return value and
+   * error code.
+   */
+  r = nbd_connect_unix (nbd, argv[1]);
+  if (r == -1 && nbd_get_errno () == ENOTSUP) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  if (nbd_get_nr_list_exports (nbd) == 0) {
+    fprintf (stderr, "Server does not support "
+             "listing exports.\n");
+    exit (EXIT_FAILURE);
+  }
+
+  /* Display the list of exports. */
+  for (i = 0;
+       i < nbd_get_nr_list_exports (nbd);
+       i++) {
+    name = nbd_get_list_export_name (nbd, i);
+    printf ("[%d] %s\n", i, name);
+    free (name);
+  }
+  printf ("Which export to connect to? ");
+  if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE);
+  name = nbd_get_list_export_name (nbd, i);
+  printf ("Connecting to %s ...\n", name);
+  nbd_close (nbd);
+
+  /* Connect again to the chosen export. */
+  nbd2 = nbd_create ();
+  if (nbd2 == NULL) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  if (nbd_set_export_name (nbd2, name) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  if (nbd_connect_unix (nbd2, argv[1]) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  /* Read the size in bytes and print it. */
+  size = nbd_get_size (nbd2);
+  if (size == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  printf ("%s: %s: size = %" PRIi64 " bytes\n",
+          argv[1], name, size);
+
+  /* Close the libnbd handle. */
+  nbd_close (nbd2);
+
+  free (name);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/generator/API.ml b/generator/API.ml
index c75b02e..8fbe815 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -588,6 +588,72 @@ the time of compilation.";
                 Link "aio_is_created"; Link
"aio_is_ready"];
   };
 
+  "set_list_exports", {
+    default_call with
+    args = [Bool "list"]; ret = RErr;
+    permitted_states = [ Created ];
+    shortdesc = "set whether to list server exports";
+    longdesc = "\
+Set this flag to true on a handle in order to list NBD exports
+provided by the server.
+
+In this mode, during connection we query the server for the list
+of NBD exports and collect them in the handle.  You can query
+the list of exports provided by the server by calling
+C<nbd_get_nr_exports> and C<nbd_get_export_name>.  After choosing
+the export you want, you should close this handle, create a new
+NBD handle (C<nbd_create>), set the export name
(C<nbd_set_export_name>),
+and connect on the new handle.
+
+Some servers do not support listing exports at all.  In
+that case the connect call will return error C<ENOTSUP>
+and C<nbd_get_nr_exports> will return 0.
+
+Some servers do not respond with all the exports they
+support, either because of an incomplete implementation of
+this feature, or because they only list exports relevant
+to non-TLS or TLS when a non-TLS or TLS connection is
+opened.";
+    example = Some "examples/list-exports.c";
+    see_also = [Link "get_list_exports";
+                Link "get_nr_list_exports"; Link
"get_list_export_name"];
+  };
+
+  "get_list_exports", {
+    default_call with
+    args = []; ret = RBool;
+    may_set_error = false;
+    shortdesc = "return whether list exports mode was enabled";
+    longdesc = "\
+Return true if list exports mode was enabled on this handle.";
+    see_also = [Link "set_list_exports"];
+  };
+
+  "get_nr_list_exports", {
+    default_call with
+    args = []; ret = RInt;
+    permitted_states = [ Closed; Dead ];
+    shortdesc = "return the number of exports returned by the
server";
+    longdesc = "\
+If list exports mode was enabled on the handle and you connected
+to the server, this returns the number of exports returned by the
+server.  This may be 0 or incomplete for reasons given in
+C<nbd_set_list_exports>.";
+    see_also = [Link "set_list_exports"];
+  };
+
+  "get_list_export_name", {
+    default_call with
+    args = [ Int "i" ]; ret = RString;
+    permitted_states = [ Closed; Dead ];
+    shortdesc = "return the i'th export name";
+    longdesc = "\
+If list exports mode was enabled on the handle and you connected
+to the server, this can be used to return the i'th export name
+from the list returned by the server.";
+    see_also = [Link "set_list_exports"];
+  };
+
   "add_meta_context", {
     default_call with
     args = [ String "name" ]; ret = RErr;
@@ -2197,6 +2263,12 @@ let first_version = [
   "set_uri_allow_tls", (1, 2);
   "set_uri_allow_local_file", (1, 2);
 
+  (* Added in 1.3.x development cycle, will be stable and supported in 1.4. *)
+  "set_list_exports", (1, 4);
+  "get_list_exports", (1, 4);
+  "get_nr_list_exports", (1, 4);
+  "get_list_export_name", (1, 4);
+
   (* These calls are proposed for a future version of libnbd, but
    * have not been added to any released version so far.
   "get_tls_certificates", (1, ??);
diff --git a/generator/Makefile.am b/generator/Makefile.am
index 0389d70..d64a953 100644
--- a/generator/Makefile.am
+++ b/generator/Makefile.am
@@ -30,6 +30,7 @@ states_code = \
 	states-issue-command.c \
 	states-magic.c \
 	states-newstyle-opt-export-name.c \
+	states-newstyle-opt-list.c \
 	states-newstyle-opt-go.c \
 	states-newstyle-opt-set-meta-context.c \
 	states-newstyle-opt-starttls.c \
diff --git a/generator/state_machine.ml b/generator/state_machine.ml
index bd65ffb..5a26a79 100644
--- a/generator/state_machine.ml
+++ b/generator/state_machine.ml
@@ -273,6 +273,7 @@ and newstyle_state_machine = [
    * state needs to run and skip to the next state in the list if not.
    *)
   Group ("OPT_STARTTLS", newstyle_opt_starttls_state_machine);
+  Group ("OPT_LIST", newstyle_opt_list_state_machine);
   Group ("OPT_STRUCTURED_REPLY",
newstyle_opt_structured_reply_state_machine);
   Group ("OPT_SET_META_CONTEXT",
newstyle_opt_set_meta_context_state_machine);
   Group ("OPT_GO", newstyle_opt_go_state_machine);
@@ -341,6 +342,44 @@ and newstyle_opt_starttls_state_machine = [
   };
 ]
 
+(* Fixed newstyle NBD_OPT_LIST option. *)
+and newstyle_opt_list_state_machine = [
+  State {
+    default_state with
+    name = "START";
+    comment = "Start listing exports if in list mode.";
+    external_events = [];
+  };
+
+  State {
+    default_state with
+    name = "SEND";
+    comment = "Send newstyle NBD_OPT_LIST to being listing exports";
+    external_events = [ NotifyWrite, "" ];
+  };
+
+  State {
+    default_state with
+    name = "RECV_REPLY";
+    comment = "Receive newstyle NBD_OPT_LIST reply";
+    external_events = [ NotifyRead, "" ];
+  };
+
+  State {
+    default_state with
+    name = "RECV_REPLY_PAYLOAD";
+    comment = "Receive any newstyle NBD_OPT_LIST reply payload";
+    external_events = [ NotifyRead, "" ];
+  };
+
+  State {
+    default_state with
+    name = "CHECK_REPLY";
+    comment = "Check newstyle NBD_OPT_LIST reply";
+    external_events = [];
+  };
+]
+
 (* Fixed newstyle NBD_OPT_STRUCTURED_REPLY option. *)
 and newstyle_opt_structured_reply_state_machine = [
   State {
diff --git a/generator/states-newstyle-opt-list.c
b/generator/states-newstyle-opt-list.c
new file mode 100644
index 0000000..3c68bb5
--- /dev/null
+++ b/generator/states-newstyle-opt-list.c
@@ -0,0 +1,131 @@
+/* nbd client library in userspace: state machine
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* State machine for sending NBD_OPT_LIST to list exports.
+ *
+ * This is skipped unless list exports mode was enabled on the handle.
+ */
+
+STATE_MACHINE {
+ NEWSTYLE.OPT_LIST.START:
+  if (!h->list_exports) {
+    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+    return 0;
+  }
+
+  h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
+  h->sbuf.option.option = htobe32 (NBD_OPT_LIST);
+  h->sbuf.option.optlen = 0;
+  h->wbuf = &h->sbuf;
+  h->wlen = sizeof (h->sbuf.option);
+  SET_NEXT_STATE (%SEND);
+  return 0;
+
+ NEWSTYLE.OPT_LIST.SEND:
+  switch (send_from_wbuf (h)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return 0;
+  case 0:
+    h->rbuf = &h->sbuf;
+    h->rlen = sizeof (h->sbuf.or.option_reply);
+    SET_NEXT_STATE (%RECV_REPLY);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_LIST.RECV_REPLY:
+  switch (recv_into_rbuf (h)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return 0;
+  case 0:
+    if (prepare_for_reply_payload (h, NBD_OPT_LIST) == -1) {
+      SET_NEXT_STATE (%.DEAD);
+      return 0;
+    }
+    SET_NEXT_STATE (%RECV_REPLY_PAYLOAD);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_LIST.RECV_REPLY_PAYLOAD:
+  switch (recv_into_rbuf (h)) {
+  case -1: SET_NEXT_STATE (%.DEAD); return 0;
+  case 0:  SET_NEXT_STATE (%CHECK_REPLY);
+  }
+  return 0;
+
+ NEWSTYLE.OPT_LIST.CHECK_REPLY:
+  const size_t maxpayload = sizeof h->sbuf.or.payload.server;
+  uint32_t reply;
+  uint32_t len;
+  uint32_t elen;
+  char *name;
+  char **new_exports;
+
+  reply = be32toh (h->sbuf.or.option_reply.reply);
+  len = be32toh (h->sbuf.or.option_reply.replylen);
+  switch (reply) {
+  case NBD_REP_SERVER:
+    /* Got one export. */
+    if (len > maxpayload)
+      debug (h, "skipping too large export name reply");
+    else {
+      elen = be32toh (h->sbuf.or.payload.server.server.export_name_len);
+      if (elen > len - 4) {
+        set_error (0, "invalid export length");
+        SET_NEXT_STATE (%.DEAD);
+        return 0;
+      }
+      /* Copy the export name to the handle list. */
+      name = strndup (h->sbuf.or.payload.server.str, elen);
+      if (name == NULL) {
+        set_error (errno, "strdup");
+        SET_NEXT_STATE (%.DEAD);
+        return 0;
+      }
+      new_exports = realloc (h->exports, sizeof (char *) *
(h->nr_exports+1));
+      if (new_exports == NULL) {
+        set_error (errno, "strdup");
+        SET_NEXT_STATE (%.DEAD);
+        free (name);
+        return 0;
+      }
+      h->exports = new_exports;
+      h->exports[h->nr_exports++] = name;
+    }
+
+    /* Wait for more replies. */
+    h->rbuf = &h->sbuf;
+    h->rlen = sizeof (h->sbuf.or.option_reply);
+    SET_NEXT_STATE (%RECV_REPLY);
+    return 0;
+
+  case NBD_REP_ACK:
+    /* Finished receiving the list. */
+    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+    return 0;
+
+  default:
+    if (handle_reply_error (h) == -1) {
+      SET_NEXT_STATE (%.DEAD);
+      return 0;
+    }
+    set_error (ENOTSUP, "unexpected response, possibly the server does not
"
+               "support listing exports");
+    SET_NEXT_STATE (%.DEAD);
+    return 0;
+  }
+  return 0;
+
+} /* END STATE MACHINE */
diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index d220c4f..2d74e5f 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -22,7 +22,7 @@ STATE_MACHINE {
  NEWSTYLE.OPT_STARTTLS.START:
   /* If TLS was not requested we skip this option and go to the next one. */
   if (h->tls == LIBNBD_TLS_DISABLE) {
-    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+    SET_NEXT_STATE (%^OPT_LIST.START);
     return 0;
   }
 
@@ -101,7 +101,7 @@ STATE_MACHINE {
     debug (h,
            "server refused TLS (%s), continuing with unencrypted
connection",
            reply == NBD_REP_ERR_POLICY ? "policy" : "not
supported");
-    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+    SET_NEXT_STATE (%^OPT_LIST.START);
     return 0;
   }
   return 0;
@@ -120,7 +120,7 @@ STATE_MACHINE {
     nbd_internal_crypto_debug_tls_enabled (h);
 
     /* Continue with option negotiation. */
-    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+    SET_NEXT_STATE (%^OPT_LIST.START);
     return 0;
   }
   /* Continue handshake. */
@@ -143,7 +143,7 @@ STATE_MACHINE {
     debug (h, "connection is using TLS");
 
     /* Continue with option negotiation. */
-    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
+    SET_NEXT_STATE (%^OPT_LIST.START);
     return 0;
   }
   /* Continue handshake. */
diff --git a/lib/handle.c b/lib/handle.c
index 88b18e3..4e93144 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -113,6 +113,7 @@ void
 nbd_close (struct nbd_handle *h)
 {
   struct meta_context *m, *m_next;
+  size_t i;
 
   nbd_internal_set_error_context ("nbd_close");
 
@@ -130,6 +131,9 @@ nbd_close (struct nbd_handle *h)
     free (m->name);
     free (m);
   }
+  for (i = 0; i < h->nr_exports; ++i)
+    free (h->exports[i]);
+  free (h->exports);
   free_cmd_list (h->cmds_to_issue);
   free_cmd_list (h->cmds_in_flight);
   free_cmd_list (h->cmds_done);
@@ -226,6 +230,52 @@ nbd_unlocked_get_export_name (struct nbd_handle *h)
   return copy;
 }
 
+int
+nbd_unlocked_set_list_exports (struct nbd_handle *h, bool list)
+{
+  h->list_exports = true;
+  return 0;
+}
+
+/* NB: may_set_error = false. */
+int
+nbd_unlocked_get_list_exports (struct nbd_handle *h)
+{
+  return h->list_exports;
+}
+
+int
+nbd_unlocked_get_nr_list_exports (struct nbd_handle *h)
+{
+  if (!h->list_exports) {
+    set_error (EINVAL, "list exports mode not selected on this
handle");
+    return -1;
+  }
+  return (int) h->nr_exports;
+}
+
+char *
+nbd_unlocked_get_list_export_name (struct nbd_handle *h,
+                                   int i)
+{
+  char *name;
+
+  if (!h->list_exports) {
+    set_error (EINVAL, "list exports mode not selected on this
handle");
+    return NULL;
+  }
+  if (i < 0 || i >= (int) h->nr_exports) {
+    set_error (EINVAL, "invalid index");
+    return NULL;
+  }
+  name = strdup (h->exports[i]);
+  if (!name) {
+    set_error (errno, "strdup");
+    return NULL;
+  }
+  return name;
+}
+
 int
 nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name)
 {
diff --git a/lib/internal.h b/lib/internal.h
index c99c3e7..2f1727f 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -93,6 +93,11 @@ struct nbd_handle {
   int uri_allow_tls;
   bool uri_allow_local_file;
 
+  /* List exports mode. */
+  bool list_exports;
+  size_t nr_exports;
+  char **exports;
+
   /* Global flags from the server. */
   uint16_t gflags;
 
@@ -158,6 +163,10 @@ struct nbd_handle {
     struct {
       struct nbd_fixed_new_option_reply option_reply;
       union {
+        struct {
+          struct nbd_fixed_new_option_reply_server server;
+          char str[NBD_MAX_STRING];
+        } __attribute__((packed)) server;
         struct nbd_fixed_new_option_reply_info_export export;
         struct {
           struct nbd_fixed_new_option_reply_meta_context context;
diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h
index df0b4c6..90fdbd2 100644
--- a/lib/nbd-protocol.h
+++ b/lib/nbd-protocol.h
@@ -154,6 +154,12 @@ struct nbd_fixed_new_option_reply_info_export {
   uint16_t eflags;              /* per-export flags */
 } NBD_ATTRIBUTE_PACKED;
 
+/* NBD_REP_SERVER reply (follows fixed_new_option_reply). */
+struct nbd_fixed_new_option_reply_server {
+  uint32_t export_name_len;     /* length of export name */
+  /* followed by a string export name and description*/
+} NBD_ATTRIBUTE_PACKED;
+
 /* NBD_REP_META_CONTEXT reply (follows fixed_new_option_reply). */
 struct nbd_fixed_new_option_reply_meta_context {
   uint32_t context_id;          /* metadata context ID */
-- 
2.27.0

On 7/20/20 9:43 AM, Richard W.M. Jones wrote:
> A major missing feature of this library was the ability to list
> exports from an NBD server.  This implements the feature by adding a
> new handle mode and additional functions for querying the list of
> export names.
> ---
>   .gitignore                               |   1 +
>   examples/Makefile.am                     |  14 +++
>   examples/list-exports.c                  | 108 +++++++++++++++++++
>   generator/API.ml                         |  72 +++++++++++++
>   generator/Makefile.am                    |   1 +
>   generator/state_machine.ml               |  39 +++++++
>   generator/states-newstyle-opt-list.c     | 131 +++++++++++++++++++++++
>   generator/states-newstyle-opt-starttls.c |   8 +-
>   lib/handle.c                             |  50 +++++++++
>   lib/internal.h                           |   9 ++
>   lib/nbd-protocol.h                       |   6 ++
>   11 files changed, 435 insertions(+), 4 deletions(-)
> 
> +++ b/examples/list-exports.c
> @@ -0,0 +1,108 @@
> +/* This example shows how to list NBD exports.
> + *
> + * To test this with qemu-nbd:
> + *   $ qemu-nbd -x "hello" -t -k /tmp/sock disk.img
> + *   $ ./run examples/list-exports /tmp/sock
> + *   [0] hello
> + *   Which export to connect to? 0
> + *   Connecting to hello ...
> + *   /tmp/sock: hello: size = 2048 bytes
> + */
It's possible to use qemu-kvm to set up multiple export names (via QMP 
commands) over one NBD server, if that would make the example any more 
powerful (although it is a lot more verbose than the one-liner of 
qemu-nbd which always has a single export).

And I'm guessing you are hoping to add counterpart APIs into nbdkit to 
expose multiple exports (such as the tar or ext2 filter being able to 
list available exports when using the client exportname...)
> +  /* Set the list exports mode in the handle. */
> +  nbd_set_list_exports (nbd, 1);
s/1/true/, given that our C bindings actually use the bool type.
> +
> +  /* Connect to the NBD server over a
> +   * Unix domain socket.  A side effect of
> +   * connecting is to list the exports.
> +   * This operation can fail normally, so
> +   * we need to check the return value and
> +   * error code.
> +   */
> +  r = nbd_connect_unix (nbd, argv[1]);
> +  if (r == -1 && nbd_get_errno () == ENOTSUP) {
> +    fprintf (stderr, "%s\n", nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  if (nbd_get_nr_list_exports (nbd) == 0) {
> +    fprintf (stderr, "Server does not support "
> +             "listing exports.\n");
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  /* Display the list of exports. */
> +  for (i = 0;
> +       i < nbd_get_nr_list_exports (nbd);
> +       i++) {
> +    name = nbd_get_list_export_name (nbd, i);
> +    printf ("[%d] %s\n", i, name);
> +    free (name);
> +  }
Looks like a pretty simple way to do the iteration.
> +  printf ("Which export to connect to? ");
> +  if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE);
scanf("%d") is undefined on integer overflow, but is simple enough for
use in this example.  In fact, for an example I might just use atoi(), 
with even worse error-handling behavior but easier use.
> +  name = nbd_get_list_export_name (nbd, i);
> +  printf ("Connecting to %s ...\n", name);
> +  nbd_close (nbd);
> +
> +  /* Connect again to the chosen export. */
> +  nbd2 = nbd_create ();
> +  if (nbd2 == NULL) {
> +    fprintf (stderr, "%s\n", nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
> +
> +  if (nbd_set_export_name (nbd2, name) == -1) {
> +    fprintf (stderr, "%s\n", nbd_get_error ());
> +    exit (EXIT_FAILURE);
> +  }
I see what you did.  Rather than letting a user re-use the connection 
(and pick an export name after obtaining the listing), you require the 
user to transfer the state of which export name is interesting from the 
first handle in list mode over to the requested export in the second 
handle.  I don't see any convenient way to do it all on one handle, so 
your approach is good enough.
> +++ b/generator/API.ml
> @@ -588,6 +588,72 @@ the time of compilation.";
>                   Link "aio_is_created"; Link
"aio_is_ready"];
>     };
>   
> +  "set_list_exports", {
> +    default_call with
> +    args = [Bool "list"]; ret = RErr;
> +    permitted_states = [ Created ];
> +    shortdesc = "set whether to list server exports";
> +    longdesc = "\
> +Set this flag to true on a handle in order to list NBD exports
> +provided by the server.
> +
> +In this mode, during connection we query the server for the list
> +of NBD exports and collect them in the handle.  You can query
> +the list of exports provided by the server by calling
> +C<nbd_get_nr_exports> and C<nbd_get_export_name>.  After
choosing
> +the export you want, you should close this handle, create a new
> +NBD handle (C<nbd_create>), set the export name
(C<nbd_set_export_name>),
> +and connect on the new handle.
> +
> +Some servers do not support listing exports at all.  In
> +that case the connect call will return error C<ENOTSUP>
> +and C<nbd_get_nr_exports> will return 0.
> +
> +Some servers do not respond with all the exports they
> +support, either because of an incomplete implementation of
> +this feature, or because they only list exports relevant
> +to non-TLS or TLS when a non-TLS or TLS connection is
> +opened.";
Here's looking at 'nbdkit info' which supports more exports than
there
are atoms in the universe ;)
> +    example = Some "examples/list-exports.c";
> +    see_also = [Link "get_list_exports";
> +                Link "get_nr_list_exports"; Link
"get_list_export_name"];
> +  };
Looks nice.
> +
> +  "get_nr_list_exports", {
> +    default_call with
> +    args = []; ret = RInt;
> +    permitted_states = [ Closed; Dead ];
> +    shortdesc = "return the number of exports returned by the
server";
> +    longdesc = "\
> +If list exports mode was enabled on the handle and you connected
> +to the server, this returns the number of exports returned by the
> +server.  This may be 0 or incomplete for reasons given in
> +C<nbd_set_list_exports>.";
> +    see_also = [Link "set_list_exports"];
> +  };
> +
> +  "get_list_export_name", {
> +    default_call with
> +    args = [ Int "i" ]; ret = RString;
> +    permitted_states = [ Closed; Dead ];
> +    shortdesc = "return the i'th export name";
> +    longdesc = "\
> +If list exports mode was enabled on the handle and you connected
> +to the server, this can be used to return the i'th export name
> +from the list returned by the server.";
> +    see_also = [Link "set_list_exports"];
> +  };
Is it worth trying to have some sort of a callback mode where the 
callback function is called once per export name advertised while the 
connection attempt is underway, rather than this post-mortem query of 
the data copied into the nbd handle?  But as your added example code 
showed, this wasn't too bad to code with.
> +++ b/generator/state_machine.ml
> @@ -273,6 +273,7 @@ and newstyle_state_machine = [
>      * state needs to run and skip to the next state in the list if not.
>      *)
>     Group ("OPT_STARTTLS", newstyle_opt_starttls_state_machine);
> +  Group ("OPT_LIST", newstyle_opt_list_state_machine);
>     Group ("OPT_STRUCTURED_REPLY",
newstyle_opt_structured_reply_state_machine);
>     Group ("OPT_SET_META_CONTEXT",
newstyle_opt_set_meta_context_state_machine);
>     Group ("OPT_GO", newstyle_opt_go_state_machine);
> @@ -341,6 +342,44 @@ and newstyle_opt_starttls_state_machine = [
>     };
>   ]
>   
> +(* Fixed newstyle NBD_OPT_LIST option. *)
> +and newstyle_opt_list_state_machine = [
> +  State {
> +    default_state with
> +    name = "START";
> +    comment = "Start listing exports if in list mode.";
> +    external_events = [];
> +  };
> +
> +  State {
> +    default_state with
> +    name = "SEND";
> +    comment = "Send newstyle NBD_OPT_LIST to being listing
exports";
s/being/begin/
> +++ b/generator/states-newstyle-opt-list.c
> +STATE_MACHINE {
> + NEWSTYLE.OPT_LIST.START:
> +  if (!h->list_exports) {
> +    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
> +    return 0;
> +  }
> +
> +  h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
> +  h->sbuf.option.option = htobe32 (NBD_OPT_LIST);
> +  h->sbuf.option.optlen = 0;
> +  h->wbuf = &h->sbuf;
> +  h->wlen = sizeof (h->sbuf.option);
> +  SET_NEXT_STATE (%SEND);
> +  return 0;
Is it worth enhancing the code to add another list mode where we can 
also query NBD_OPT_INFO on each export thus listed?  (Compare to 
'qemu-nbd --list').  Hmm, if we do that, list mode may need to be an 
enum instead of a bool.  But that would be a followup because it adds 
more complexity; this is already a good addition on its own.
> + NEWSTYLE.OPT_LIST.CHECK_REPLY:
> +  const size_t maxpayload = sizeof h->sbuf.or.payload.server;
> +  uint32_t reply;
> +  uint32_t len;
> +  uint32_t elen;
> +  char *name;
> +  char **new_exports;
> +
> +  reply = be32toh (h->sbuf.or.option_reply.reply);
> +  len = be32toh (h->sbuf.or.option_reply.replylen);
> +  switch (reply) {
> +  case NBD_REP_SERVER:
> +    /* Got one export. */
> +    if (len > maxpayload)
> +      debug (h, "skipping too large export name reply");
> +    else {
> +      elen = be32toh
(h->sbuf.or.payload.server.server.export_name_len);
> +      if (elen > len - 4) {
> +        set_error (0, "invalid export length");
> +        SET_NEXT_STATE (%.DEAD);
> +        return 0;
> +      }
> +      /* Copy the export name to the handle list. */
> +      name = strndup (h->sbuf.or.payload.server.str, elen);
> +      if (name == NULL) {
> +        set_error (errno, "strdup");
> +        SET_NEXT_STATE (%.DEAD);
> +        return 0;
> +      }
> +      new_exports = realloc (h->exports, sizeof (char *) *
(h->nr_exports+1));
> +      if (new_exports == NULL) {
> +        set_error (errno, "strdup");
> +        SET_NEXT_STATE (%.DEAD);
> +        free (name);
> +        return 0;
> +      }
> +      h->exports = new_exports;
> +      h->exports[h->nr_exports++] = name;
> +    }
> +
> +    /* Wait for more replies. */
> +    h->rbuf = &h->sbuf;
> +    h->rlen = sizeof (h->sbuf.or.option_reply);
> +    SET_NEXT_STATE (%RECV_REPLY);
> +    return 0;
> +
> +  case NBD_REP_ACK:
> +    /* Finished receiving the list. */
> +    SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
> +    return 0;
> +
> +  default:
> +    if (handle_reply_error (h) == -1) {
> +      SET_NEXT_STATE (%.DEAD);
Would this be better as %.CLOSED, or even a transition into a state 
where we send a clean NBD_OPT_ABORT for graceful shutdown rather than 
abrupt disconnect from the server?
> +      return 0;
> +    }
> +    set_error (ENOTSUP, "unexpected response, possibly the server
does not "
> +               "support listing exports");
> +    SET_NEXT_STATE (%.DEAD);
whereas this definitely makes sense as %.DEAD.
> +    return 0;
> +  }
> +  return 0;
> +
> +} /* END STATE MACHINE */
> diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
> index d220c4f..2d74e5f 100644
> --- a/generator/states-newstyle-opt-starttls.c
> +++ b/lib/handle.c
> +int
> +nbd_unlocked_set_list_exports (struct nbd_handle *h, bool list)
> +{
> +  h->list_exports = true;
s/true/list/ (you never really tested clearing the mode...)

> +char *
> +nbd_unlocked_get_list_export_name (struct nbd_handle *h,
> +                                   int i)
> +{
> +  char *name;
> +
> +  if (!h->list_exports) {
> +    set_error (EINVAL, "list exports mode not selected on this
handle");
> +    return NULL;
> +  }
> +  if (i < 0 || i >= (int) h->nr_exports) {
That cast makes sense, but looks odd - any server that tries to send 
more than 2G export names (by assuming that our size_t > 32 bits) is 
sending a LOT of traffic in response to a single NBD_OPT_LIST command. 
Would it be better to just track the list size as int, and change the 
state machine to fail if the server hasn't completed its listing in X 
number of responses (perhaps where X is 1 million, or determined by the 
user, or...)?

> +++ b/lib/nbd-protocol.h
> @@ -154,6 +154,12 @@ struct nbd_fixed_new_option_reply_info_export {
>     uint16_t eflags;              /* per-export flags */
>   } NBD_ATTRIBUTE_PACKED;
>   
> +/* NBD_REP_SERVER reply (follows fixed_new_option_reply). */
> +struct nbd_fixed_new_option_reply_server {
> +  uint32_t export_name_len;     /* length of export name */
> +  /* followed by a string export name and description*/
> +} NBD_ATTRIBUTE_PACKED;
> +
>   /* NBD_REP_META_CONTEXT reply (follows fixed_new_option_reply). */
>   struct nbd_fixed_new_option_reply_meta_context {
>     uint32_t context_id;          /* metadata context ID */
> 
We'll want this synchronized with nbdkit.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org