Libguestfs - Jul 2020 - [libnbd PATCH 2/2] info: Expose description in list mode

An incremental improvement on top of listing exports.  I still think
it's worth experimenting with revisiting how our API for list mode
should actually work [1] (so that we can reuse a single connection for
both grabbing the list and finally using NBD_OPT_GO), but this change
was easier to whip together while still thinking about that.

[1] https://www.redhat.com/archives/libguestfs/2020-July/msg00092.html

Eric Blake (2):
  api: Get description alongside name from NBD_REP_SERVER
  info: Expose description in list mode

 lib/internal.h                       |  7 +++++-
 generator/API.ml                     | 21 ++++++++++++++++--
 generator/states-newstyle-opt-list.c | 29 +++++++++++++++++--------
 lib/handle.c                         | 32 ++++++++++++++++++++++++----
 examples/list-exports.c              |  6 +++++-
 interop/Makefile.am                  |  6 ++++--
 interop/list-exports.c               | 12 +++++++++--
 info/info-list-json.sh               |  3 ++-
 info/info-list.sh                    |  3 ++-
 info/nbdinfo.c                       | 21 +++++++++++++-----
 10 files changed, 112 insertions(+), 28 deletions(-)

-- 
2.27.0

At least 'qemu-nbd -D' can send a description when listing an export;
it's not that much harder to expose things via a new
nbd_get_list_export_description.  We may still want to rethink our
list mode API (perhaps adding a callback that informs the client of
name/description pairs as they come in, instead of malloc'ing a copy
for browsing later), but for this patch, it is a straightforward
expansion building on top of commit c2851cf5.
---
 lib/internal.h                       |  7 +++++-
 generator/API.ml                     | 21 ++++++++++++++++--
 generator/states-newstyle-opt-list.c | 29 +++++++++++++++++--------
 lib/handle.c                         | 32 ++++++++++++++++++++++++----
 examples/list-exports.c              |  6 +++++-
 interop/Makefile.am                  |  6 ++++--
 interop/list-exports.c               | 12 +++++++++--
 7 files changed, 92 insertions(+), 21 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index 90bc94e..4d0c4e1 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -66,6 +66,11 @@ struct meta_context;
 struct socket;
 struct command;

+struct export {
+  char *name;
+  char *description;
+};
+
 struct nbd_handle {
   /* Unique name assigned to this handle for debug messages
    * (to avoid having to print actual pointers).
@@ -96,7 +101,7 @@ struct nbd_handle {
   /* List exports mode. */
   bool list_exports;
   size_t nr_exports;
-  char **exports;
+  struct export *exports;

   /* Global flags from the server. */
   uint16_t gflags;
diff --git a/generator/API.ml b/generator/API.ml
index 985d715..3b4cbb1 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -656,7 +656,8 @@ 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"];
+    see_also = [Link "set_list_exports"; Link
"get_list_export_name";
+                Link "get_list_export_description"];
   };

   "get_list_export_name", {
@@ -668,7 +669,22 @@ C<nbd_set_list_exports>.";
 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"];
+    see_also = [Link "set_list_exports"; Link
"get_list_export_description"];
+  };
+
+  "get_list_export_description", {
+    default_call with
+    args = [ Int "i" ]; ret = RString;
+    permitted_states = [ Closed; Dead ];
+    shortdesc = "return the i'th export description";
+    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 description
+from the list returned by the server, which may be an empty string.
+
+Many servers omit a description.  For L<qemu-nbd(8)>, a description
+is set with I<-D>.";
+    see_also = [Link "set_list_exports"; Link
"get_list_export_name"];
   };

   "add_meta_context", {
@@ -2336,6 +2352,7 @@ let first_version = [
   "get_list_exports", (1, 4);
   "get_nr_list_exports", (1, 4);
   "get_list_export_name", (1, 4);
+  "get_list_export_description", (1, 4);
   "get_block_size", (1, 4);

   (* These calls are proposed for a future version of libnbd, but
diff --git a/generator/states-newstyle-opt-list.c
b/generator/states-newstyle-opt-list.c
index 49b6f75..f7ea4dc 100644
--- a/generator/states-newstyle-opt-list.c
+++ b/generator/states-newstyle-opt-list.c
@@ -70,8 +70,8 @@ STATE_MACHINE {
   uint32_t reply;
   uint32_t len;
   uint32_t elen;
-  char *name;
-  char **new_exports;
+  struct export exp;
+  struct export *new_exports;

   reply = be32toh (h->sbuf.or.option_reply.reply);
   len = be32toh (h->sbuf.or.option_reply.replylen);
@@ -82,27 +82,38 @@ STATE_MACHINE {
       debug (h, "skipping too large export name reply");
     else {
       elen = be32toh (h->sbuf.or.payload.server.server.export_name_len);
-      if (elen > len - 4 || elen > NBD_MAX_STRING) {
+      if (elen > len - 4 || elen > NBD_MAX_STRING ||
+          len - 4 - elen > NBD_MAX_STRING) {
         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) {
+      /* Copy the export name and description to the handle list. */
+      exp.name = strndup (h->sbuf.or.payload.server.str, elen);
+      if (exp.name == NULL) {
         set_error (errno, "strdup");
         SET_NEXT_STATE (%.DEAD);
         return 0;
       }
-      new_exports = realloc (h->exports, sizeof (char *) *
(h->nr_exports+1));
+      exp.description = strndup (h->sbuf.or.payload.server.str + elen,
+                                 len - 4 - elen);
+      if (exp.name == NULL) {
+        set_error (errno, "strdup");
+        free (exp.name);
+        SET_NEXT_STATE (%.DEAD);
+        return 0;
+      }
+      new_exports = realloc (h->exports,
+                             sizeof (*new_exports) * (h->nr_exports+1));
       if (new_exports == NULL) {
         set_error (errno, "strdup");
         SET_NEXT_STATE (%.DEAD);
-        free (name);
+        free (exp.name);
+        free (exp.description);
         return 0;
       }
       h->exports = new_exports;
-      h->exports[h->nr_exports++] = name;
+      h->exports[h->nr_exports++] = exp;
     }

     /* Just limit this so we don't receive unlimited amounts
diff --git a/lib/handle.c b/lib/handle.c
index 5ac052a..210ac7d 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * 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
@@ -131,8 +131,10 @@ nbd_close (struct nbd_handle *h)
     free (m->name);
     free (m);
   }
-  for (i = 0; i < h->nr_exports; ++i)
-    free (h->exports[i]);
+  for (i = 0; i < h->nr_exports; ++i) {
+    free (h->exports[i].name);
+    free (h->exports[i].description);
+  }
   free (h->exports);
   free_cmd_list (h->cmds_to_issue);
   free_cmd_list (h->cmds_in_flight);
@@ -268,7 +270,7 @@ nbd_unlocked_get_list_export_name (struct nbd_handle *h,
     set_error (EINVAL, "invalid index");
     return NULL;
   }
-  name = strdup (h->exports[i]);
+  name = strdup (h->exports[i].name);
   if (!name) {
     set_error (errno, "strdup");
     return NULL;
@@ -276,6 +278,28 @@ nbd_unlocked_get_list_export_name (struct nbd_handle *h,
   return name;
 }

+char *
+nbd_unlocked_get_list_export_description (struct nbd_handle *h,
+                                          int i)
+{
+  char *desc;
+
+  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;
+  }
+  desc = strdup (h->exports[i].description);
+  if (!desc) {
+    set_error (errno, "strdup");
+    return NULL;
+  }
+  return desc;
+}
+
 int
 nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name)
 {
diff --git a/examples/list-exports.c b/examples/list-exports.c
index f654f39..643e611 100644
--- a/examples/list-exports.c
+++ b/examples/list-exports.c
@@ -22,7 +22,7 @@ main (int argc, char *argv[])
 {
   struct nbd_handle *nbd, *nbd2;
   int r, i;
-  char *name;
+  char *name, *desc;
   int64_t size;

   if (argc != 2) {
@@ -65,7 +65,11 @@ main (int argc, char *argv[])
        i++) {
     name = nbd_get_list_export_name (nbd, i);
     printf ("[%d] %s\n", i, name);
+    desc = nbd_get_list_export_description (nbd, i);
+    if (desc && *desc)
+      printf("  (%s)\n", desc);
     free (name);
+    free (desc);
   }
   printf ("Which export to connect to? ");
   if (scanf ("%d", &i) != 1) exit (EXIT_FAILURE);
diff --git a/interop/Makefile.am b/interop/Makefile.am
index 08a4336..8b5b03c 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -1,5 +1,5 @@
 # nbd client library in userspace
-# Copyright (C) 2013-2019 Red Hat Inc.
+# 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
@@ -137,6 +137,7 @@ list_exports_nbd_server_CPPFLAGS = \
 	-DSERVER=\"$(NBD_SERVER)\" \
 	-DSERVER_PARAMS='"-C", "list-exports-nbd-config",
"-d", "0"' \
 	-DEXPORTS='"disk1", "disk2"' \
+	-DDESCRIPTIONS='"", ""' \
 	$(NULL)
 list_exports_nbd_server_CFLAGS = $(WARNINGS_CFLAGS)
 list_exports_nbd_server_LDADD = $(top_builddir)/lib/libnbd.la
@@ -146,8 +147,9 @@ list_exports_qemu_nbd_CPPFLAGS = \
 	-I$(top_srcdir)/include \
 	-DSOCKET_ACTIVATION=1 \
 	-DSERVER=\"$(QEMU_NBD)\" \
-	-DSERVER_PARAMS='"-f", "raw", "-x",
"testing", tmpfile' \
+	-DSERVER_PARAMS='"-f", "raw", "-x",
"testing", "-D", "data", tmpfile' \
 	-DEXPORTS='"testing"' \
+	-DDESCRIPTIONS='"data"' \
 	$(NULL)
 list_exports_qemu_nbd_CFLAGS = $(WARNINGS_CFLAGS)
 list_exports_qemu_nbd_LDADD = $(top_builddir)/lib/libnbd.la
diff --git a/interop/list-exports.c b/interop/list-exports.c
index e9ee0c5..d003ce9 100644
--- a/interop/list-exports.c
+++ b/interop/list-exports.c
@@ -34,7 +34,7 @@ main (int argc, char *argv[])
   char tmpfile[] = "/tmp/nbdXXXXXX";
   int fd, r;
   size_t i;
-  char *name;
+  char *name, *desc;

   /* Create a sparse temporary file. */
   fd = mkstemp (tmpfile);
@@ -71,6 +71,7 @@ main (int argc, char *argv[])

   /* Check for expected number of exports. */
   const char *exports[] = { EXPORTS };
+  const char *descriptions[] = { DESCRIPTIONS };
   const size_t nr_exports = sizeof exports / sizeof exports[0];
   r = nbd_get_nr_list_exports (nbd);
   if (r != nr_exports) {
@@ -79,7 +80,7 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  /* Check the export names. */
+  /* Check the export names and descriptions. */
   for (i = 0; i < nr_exports; ++i) {
     name = nbd_get_list_export_name (nbd, (int) i);
     if (strcmp (name, exports[i]) != 0) {
@@ -88,6 +89,13 @@ main (int argc, char *argv[])
       exit (EXIT_FAILURE);
     }
     free (name);
+    desc = nbd_get_list_export_description (nbd, (int) i);
+    if (strcmp (desc, descriptions[i]) != 0) {
+      fprintf (stderr, "%s: expected description \"%s\", but got
\"%s\"\n",
+               argv[0], descriptions[i], desc);
+      exit (EXIT_FAILURE);
+    }
+    free (desc);
   }

   nbd_close (nbd);
-- 
2.27.0

Exposing a description in single mode is harder: we'd have to request
NBD_INFO_DESCRIPTION during NBD_OPT_GO.  For now, the API only
supports descriptions in list mode.
---
 info/info-list-json.sh |  3 ++-
 info/info-list.sh      |  3 ++-
 info/nbdinfo.c         | 21 ++++++++++++++++-----
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/info/info-list-json.sh b/info/info-list-json.sh
index 3845875..af29064 100755
--- a/info/info-list-json.sh
+++ b/info/info-list-json.sh
@@ -33,7 +33,7 @@ cleanup_fn rm -f $img $out $pid $sock
 rm -f $img $out $pid $sock

 truncate -s 1M $img
-qemu-nbd -t --socket=$sock --pid-file=$pid -x "hello" $img &
+qemu-nbd -t --socket=$sock --pid-file=$pid -x "hello" -D
"world" $img &
 cleanup_fn kill $!

 # Wait for qemu-nbd to start up.
@@ -52,4 +52,5 @@ $VG nbdinfo "nbd+unix://?socket=$sock" --list --json
> $out
 cat $out

 grep '"export-name": "hello"' $out
+grep '"description": "world"' $out
 grep '"export-size": 1048576' $out
diff --git a/info/info-list.sh b/info/info-list.sh
index c30f258..d51a6ff 100755
--- a/info/info-list.sh
+++ b/info/info-list.sh
@@ -33,7 +33,7 @@ cleanup_fn rm -f $img $out $pid $sock
 rm -f $img $out $pid $sock

 truncate -s 1M $img
-qemu-nbd -t --socket=$sock --pid-file=$pid -x "hello" $img &
+qemu-nbd -t --socket=$sock --pid-file=$pid -x "hello" -D
"world" $img &
 cleanup_fn kill $!

 # Wait for qemu-nbd to start up.
@@ -52,4 +52,5 @@ $VG nbdinfo "nbd+unix://?socket=$sock" --list >
$out
 cat $out

 grep 'export="hello":' $out
+grep 'description: world' $out
 grep 'export-size: 1048576' $out
diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index 49c97be..1aca548 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -37,7 +37,7 @@ static bool probe_content, content_flag, no_content_flag;
 static bool json_output = false;
 static bool size_only = false;

-static void list_one_export (struct nbd_handle *nbd);
+static void list_one_export (struct nbd_handle *nbd, const char *desc);
 static void list_all_exports (struct nbd_handle *nbd1, const char *uri);
 static void print_json_string (const char *);
 static char *get_content (struct nbd_handle *, int64_t size);
@@ -243,7 +243,8 @@ main (int argc, char *argv[])
     }

     if (!list_all)
-      list_one_export (nbd);
+      /* XXX We would need libnbd to request NBD_INFO_DESCRIPTION */
+      list_one_export (nbd, NULL);
     else
       list_all_exports (nbd, argv[optind]);

@@ -256,7 +257,7 @@ main (int argc, char *argv[])
 }

 static void
-list_one_export (struct nbd_handle *nbd)
+list_one_export (struct nbd_handle *nbd, const char *desc)
 {
   int64_t size;
   char *export_name = NULL;
@@ -298,6 +299,8 @@ list_one_export (struct nbd_handle *nbd)
     /* Might as well use the JSON function to get an escaped string here ... */
     print_json_string (export_name);
     printf (":\n");
+    if (desc && *desc)
+      printf ("\tdescription: %s\n", desc);
     printf ("\texport-size: %" PRIi64 "\n", size);
     if (content)
       printf ("\tcontent: %s\n", content);
@@ -336,6 +339,12 @@ list_one_export (struct nbd_handle *nbd)
     print_json_string (export_name);
     printf (",\n");

+    if (desc && *desc) {
+      printf ("\t\"description\": ");
+      print_json_string (desc);
+      printf (",\n");
+    }
+
     if (content) {
       printf ("\t\"content\": ");
       print_json_string (content);
@@ -402,7 +411,7 @@ list_all_exports (struct nbd_handle *nbd1, const char *uri)
   xmlURIPtr xmluri = NULL;

   for (i = 0; i < nbd_get_nr_list_exports (nbd1); ++i) {
-    char *name, *new_path, *new_uri;
+    char *name, *desc, *new_path, *new_uri;
     struct nbd_handle *nbd2;

     name = nbd_get_list_export_name (nbd1, i);
@@ -437,10 +446,12 @@ list_all_exports (struct nbd_handle *nbd1, const char
*uri)
       }

       /* List the metadata of this export. */
-      list_one_export (nbd2);
+      desc = nbd_get_list_export_description (nbd1, i);
+      list_one_export (nbd2, desc);

       nbd_close (nbd2);
       free (new_uri);
+      free (desc);
       xmlFreeURI (xmluri); /* this also frees xmluri->path == new_path */
     }
     free (name);
-- 
2.27.0

Looks good to me, ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/