Libguestfs - Aug 2020 - [libnbd PATCH 0/4] More nbdinfo fixes

This rounds up the remaining bugs that I originally identified in:
https://www.redhat.com/archives/libguestfs/2020-July/msg00153.html

Eric Blake (4):
  api: Permit export list APIs when Connected
  info: Support --list with serializing servers
  info: Fix --json output when list size != 1
  info: Permit --size --json

 generator/API.ml       |  6 +++---
 info/info-list-json.sh |  9 +++++++++
 info/info-list.sh      |  9 +++++++++
 info/nbdinfo.c         | 41 ++++++++++++++++++++++++++++-------------
 4 files changed, 49 insertions(+), 16 deletions(-)

-- 
2.28.0

Our nbd_can_FOO APIs are allowed when Connected and Closed, but not
when Dead (which makes sense: they tell what the server answered while
connecting, and are valid whether we are still talking to the server
or cleanly disconnected, but if we are in Dead, we cannot tell if the
server answered us).  But our recent addition of list export APIs were
originally only allowed when Closed or Dead.  Dead makes sense here
(that is our default state if NBD_OPT_GO fails, but failure is likely
if the server did not like our export name; but the point of listing
was to figure out what export name to pass), but we also need to
permit Connected (because the list is accurate even if we did manage
to successfully connect).

The testsuite changes demonstrate why this change is needed (changing
just the tests without the generator causes the tests to fail), but
also highlights another issue: we are holding open two connections at
once to the same server, which deadlocks if the server in question
only permits one client at a time.
---
 generator/API.ml       |  6 +++---
 info/info-list-json.sh | 11 ++++++++++-
 info/info-list.sh      | 11 ++++++++++-
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 3ba7223..82fdf75 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -649,7 +649,7 @@ Return true if list exports mode was enabled on this
handle.";
   "get_nr_list_exports", {
     default_call with
     args = []; ret = RInt;
-    permitted_states = [ Closed; Dead ];
+    permitted_states = [ Connected; 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
@@ -663,7 +663,7 @@ C<nbd_set_list_exports>.";
   "get_list_export_name", {
     default_call with
     args = [ Int "i" ]; ret = RString;
-    permitted_states = [ Closed; Dead ];
+    permitted_states = [ Connected; Closed; Dead ];
     shortdesc = "return the i'th export name";
     longdesc = "\
 If list exports mode was enabled on the handle and you connected
@@ -675,7 +675,7 @@ from the list returned by the server.";
   "get_list_export_description", {
     default_call with
     args = [ Int "i" ]; ret = RString;
-    permitted_states = [ Closed; Dead ];
+    permitted_states = [ Connected; Closed; Dead ];
     shortdesc = "return the i'th export description";
     longdesc = "\
 If list exports mode was enabled on the handle and you connected
diff --git a/info/info-list-json.sh b/info/info-list-json.sh
index 8a07dcc..1cafb39 100755
--- a/info/info-list-json.sh
+++ b/info/info-list-json.sh
@@ -34,7 +34,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" -D
"world" $img &
+qemu-nbd -e2 -t --socket=$sock --pid-file=$pid -x "hello" -D
"world" $img &
 cleanup_fn kill $!

 # Wait for qemu-nbd to start up.
@@ -49,9 +49,18 @@ if ! test -f $pid; then
     exit 1
 fi

+# Test twice, once with an export name not on the list,...
 $VG nbdinfo "nbd+unix://?socket=$sock" --list --json > $out
 jq . < $out

 grep '"export-name": "hello"' $out
 grep '"description": "world"' $out
 grep '"export-size": 1048576' $out
+
+# ...and again with the export name included
+$VG nbdinfo "nbd+unix:///hello?socket=$sock" --list --json > $out
+jq . < $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 d51a6ff..62e5724 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" -D
"world" $img &
+qemu-nbd -e2 -t --socket=$sock --pid-file=$pid -x "hello" -D
"world" $img &
 cleanup_fn kill $!

 # Wait for qemu-nbd to start up.
@@ -48,9 +48,18 @@ if ! test -f $pid; then
     exit 1
 fi

+# Test twice, once with an export name not on the list,...
 $VG nbdinfo "nbd+unix://?socket=$sock" --list > $out
 cat $out

 grep 'export="hello":' $out
 grep 'description: world' $out
 grep 'export-size: 1048576' $out
+
+# ...and again with the export name included
+$VG nbdinfo "nbd+unix:///hello?socket=$sock" --list > $out
+cat $out
+
+grep 'export="hello":' $out
+grep 'description: world' $out
+grep 'export-size: 1048576' $out
-- 
2.28.0

Make sure the initial handle is closed if it is not already dead,
before attempting to open further handles, to avoid deadlocks if the
server serializes connections.  Reverts the hack of needing 'qemu-nbd
-e2' to avoid deadlock in the previous patch.
---
 info/info-list-json.sh | 2 +-
 info/info-list.sh      | 2 +-
 info/nbdinfo.c         | 6 ++++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/info/info-list-json.sh b/info/info-list-json.sh
index 1cafb39..fcba0dd 100755
--- a/info/info-list-json.sh
+++ b/info/info-list-json.sh
@@ -34,7 +34,7 @@ cleanup_fn rm -f $img $out $pid $sock
 rm -f $img $out $pid $sock

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

 # Wait for qemu-nbd to start up.
diff --git a/info/info-list.sh b/info/info-list.sh
index 62e5724..3550adc 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 -e2 -t --socket=$sock --pid-file=$pid -x "hello" -D
"world" $img &
+qemu-nbd -t --socket=$sock --pid-file=$pid -x "hello" -D
"world" $img &
 cleanup_fn kill $!

 # Wait for qemu-nbd to start up.
diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index 1aca548..b18cebc 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -222,6 +222,12 @@ main (int argc, char *argv[])
     protocol = nbd_get_protocol (nbd);
     tls_negotiated = nbd_get_tls_negotiated (nbd);

+    /* Disconnect from the server to move the handle into a closed
+     * state, in case the server serializes further connections; but
+     * ignore errors as the connection may already be dead.
+     */
+    nbd_shutdown (nbd, 0);
+
     if (!json_output) {
       if (protocol) {
         printf ("protocol: %s", protocol);
-- 
2.28.0

We were producing invalid JSON for a server advertising zero exports
(we left a dangling trailing comma after the "TLS" line), or for
multiple exports (we omitted commas between array elements of
"exports").  We were also failing to check for failure in
nbd_get_nr_list_exports (which was fixed a couple patches ago, but
previously possible when querying with a URI matching an export known
by the server which resulted in a handle in Connected state).

Tested by using qemu-kvm in a second terminal (although the setup is
involved enough that I did not add it to the testsuite; instead, I
will add things when nbdkit is also taught to expose list size != 1):

t1$ touch a b
t1$ qemu-kvm -nodefaults -nographic -qmp stdio
{'execute':'qmp_capabilities'}
{'execute':'nbd-server-start','arguments':{'addr':{'type':'inet',
 'data':{'host':'localhost','port':'10809'}}}}
{'execute':'blockdev-add','arguments':{'driver':'file','node-name':'a',
 'filename':'a'}}
{'execute':'blockdev-add','arguments':{'driver':'file','node-name':'b',
 'filename':'b'}}

# now qemu is advertising zero exports
t2$ nbdinfo --list --json nbd://localhost:10809 | jq .

# back to t1
{'execute':'nbd-server-add','arguments':{'device':'a'}}
{'execute':'nbd-server-add','arguments':{'device':'b'}}

# now qemu is advertising two exports
t2$ nbdinfo --list --json nbd://localhost:10809 | jq .

# back to t1
{'execute':'quit'}
---
 info/nbdinfo.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index b18cebc..f37ffea 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -37,7 +37,8 @@ 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, const char *desc);
+static void list_one_export (struct nbd_handle *nbd, const char *desc,
+                             bool first, bool last);
 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);
@@ -250,7 +251,7 @@ main (int argc, char *argv[])

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

@@ -263,7 +264,8 @@ main (int argc, char *argv[])
 }

 static void
-list_one_export (struct nbd_handle *nbd, const char *desc)
+list_one_export (struct nbd_handle *nbd, const char *desc,
+                 bool first, bool last)
 {
   int64_t size;
   char *export_name = NULL;
@@ -338,7 +340,8 @@ list_one_export (struct nbd_handle *nbd, const char *desc)
       printf ("\t%s: %" PRId64 "\n",
"block_size_maximum", block_maximum);
   }
   else {
-    printf ("\"exports\": [\n");
+    if (first)
+      printf ("\"exports\": [\n");
     printf ("\t{\n");

     printf ("\t\"export-name\": ");
@@ -399,8 +402,10 @@ list_one_export (struct nbd_handle *nbd, const char *desc)
     /* Put this one at the end because of the stupid comma thing in JSON. */
     printf ("\t\"export-size\": %" PRIi64 "\n",
size);

-    printf ("\t}\n");
-    printf ("]\n");
+    if (last)
+      printf ("\t} ]\n");
+    else
+      printf ("\t},\n");
   }

   free (content);
@@ -415,8 +420,17 @@ list_all_exports (struct nbd_handle *nbd1, const char *uri)
 {
   int i;
   xmlURIPtr xmluri = NULL;
+  int count = nbd_get_nr_list_exports (nbd1);

-  for (i = 0; i < nbd_get_nr_list_exports (nbd1); ++i) {
+  if (count == -1) {
+    fprintf (stderr, "unable to obtain list of exports: %s\n",
+             nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (count == 0 && json_output)
+    printf ("\t\"exports\": []\n");
+
+  for (i = 0; i < count; ++i) {
     char *name, *desc, *new_path, *new_uri;
     struct nbd_handle *nbd2;

@@ -453,7 +467,7 @@ list_all_exports (struct nbd_handle *nbd1, const char *uri)

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

       nbd_close (nbd2);
       free (new_uri);
-- 
2.28.0

A single integer IS a valid JSON document, after all, at least
according to RFC8259.
---
 info/nbdinfo.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/info/nbdinfo.c b/info/nbdinfo.c
index f37ffea..2e0bf27 100644
--- a/info/nbdinfo.c
+++ b/info/nbdinfo.c
@@ -158,11 +158,6 @@ main (int argc, char *argv[])
     usage (stderr, EXIT_FAILURE);

   /* You can combine certain options. */
-  if (json_output && size_only) {
-    fprintf (stderr, "%s: you cannot use %s and %s together.\n",
-             argv[0], "--json", "--size");
-    exit (EXIT_FAILURE);
-  }
   if (list_all && size_only) {
     fprintf (stderr, "%s: you cannot use %s and %s together.\n",
              argv[0], "--list", "--size");
-- 
2.28.0

ACK series, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top