Libguestfs - Jul 2020 - [libnbd PATCH] generator: Trace return even when in wrong state

Place the tracing of return values after any generated 'out:' label,
as it is important to see even the errors caused by calling a function
in the wrong handle state.

Fixes: 9df088bfa9
---

Multiple bugs found today, but only time to submit one patch so far.
I found them all while trying to implement .list_exports in nbdkit.

Bug 1: 'nbdinfo --list --json $uri' produces invalid JSON if there are
0 or more than 1 exports.  We need to move the '"exports":['
and ']'
into list_all_exports(), not list_one_export(), and manage correct
trailing comma output.  (We only tested nbdinfo with qemu-nbd, which
only lists one export, explaining how we missed this)

Bug 2: nbd_get_nr_list_exports() fails if the handle is still in READY
state, even though there were exports listed and available in that
case.  I see no reason to forbid this during READY; but I've also
mentioned that there are probably more changes coming to the way we do
list export handling in libnbd to allow the reuse of a single handle
rather than our current hacks of one handle per export.  (We only
tested nbdinfo with qemu-nbd, which generally does NOT allow
NBD_OPT_GO on the '' export if it was advertising some other export;
nbdkit is more forgiving, explaining why I hit a case where nbdinfo
was calling this function in the wrong state)

Bug 3: this patch.  When a function fails due to use in the wrong
state, the debug output was useless in telling me why.

 generator/C.ml | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index 4a0b45d..2c62d4e 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -542,13 +542,13 @@ let generate_lib_api_c ()      pr "  ret =
nbd_unlocked_%s " name;
     print_arg_list ~wrap:true ~types:false ~handle:true args optargs;
     pr ";\n";
-    if may_set_error then (
-      pr "\n";
-      print_trace_leave ret;
-      pr "\n"
-    );
+    pr "\n";
     if !need_out_label then
       pr " out:\n";
+    if may_set_error then (
+      print_trace_leave ret;
+      pr "\n"
+    );
     if is_locked then (
       pr "  if (h->public_state != get_next_state (h))\n";
       pr "    h->public_state = get_next_state (h);\n";
-- 
2.28.0

On Fri, Jul 31, 2020 at 05:11:38PM -0500, Eric Blake
wrote:
> Place the tracing of return values after any generated 'out:'
label,
> as it is important to see even the errors caused by calling a function
> in the wrong handle state.
> 
> Fixes: 9df088bfa9
> ---
> 
> Multiple bugs found today, but only time to submit one patch so far.
> I found them all while trying to implement .list_exports in nbdkit.
> 
> Bug 1: 'nbdinfo --list --json $uri' produces invalid JSON if there
are
> 0 or more than 1 exports.  We need to move the
'"exports":[' and ']'
> into list_all_exports(), not list_one_export(), and manage correct
> trailing comma output.  (We only tested nbdinfo with qemu-nbd, which
> only lists one export, explaining how we missed this)
> 
> Bug 2: nbd_get_nr_list_exports() fails if the handle is still in READY
> state, even though there were exports listed and available in that
> case.  I see no reason to forbid this during READY; but I've also
> mentioned that there are probably more changes coming to the way we do
> list export handling in libnbd to allow the reuse of a single handle
> rather than our current hacks of one handle per export.  (We only
> tested nbdinfo with qemu-nbd, which generally does NOT allow
> NBD_OPT_GO on the '' export if it was advertising some other
export;
> nbdkit is more forgiving, explaining why I hit a case where nbdinfo
> was calling this function in the wrong state)
> 
> Bug 3: this patch.  When a function fails due to use in the wrong
> state, the debug output was useless in telling me why.
> 
>  generator/C.ml | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index 4a0b45d..2c62d4e 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -542,13 +542,13 @@ let generate_lib_api_c () >      pr "  ret =
nbd_unlocked_%s " name;
>      print_arg_list ~wrap:true ~types:false ~handle:true args optargs;
>      pr ";\n";
> -    if may_set_error then (
> -      pr "\n";
> -      print_trace_leave ret;
> -      pr "\n"
> -    );
> +    pr "\n";
>      if !need_out_label then
>        pr " out:\n";
> +    if may_set_error then (
> +      print_trace_leave ret;
> +      pr "\n"
> +    );
>      if is_locked then (
>        pr "  if (h->public_state != get_next_state (h))\n";
>        pr "    h->public_state = get_next_state (h);\n";
ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW