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
Eric Blake
2020-Aug-03 18:45 UTC
[Libguestfs] [libnbd PATCH 1/4] api: Permit export list APIs when Connected
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
Eric Blake
2020-Aug-03 18:45 UTC
[Libguestfs] [libnbd PATCH 2/4] info: Support --list with serializing servers
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
Eric Blake
2020-Aug-03 18:45 UTC
[Libguestfs] [libnbd PATCH 3/4] info: Fix --json output when list size != 1
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
Eric Blake
2020-Aug-03 18:45 UTC
[Libguestfs] [libnbd PATCH 4/4] info: Permit --size --json
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
Richard W.M. Jones
2020-Aug-03 22:05 UTC
Re: [Libguestfs] [libnbd PATCH 4/4] info: Permit --size --json
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
Reasonably Related Threads
- [PATCH nbdinfo v2] info: Add a --map option for displaying allocation metadata.
- [libnbd PATCH v2 2/2] info: List available meta-contexts
- [PATCH libnbd] info: Write output atomically.
- [libnbd PATCH] info: Keep request within 4G bound
- [libnbd PATCH 2/2] info: Expose description in list mode