Libguestfs - Jun 2023 - [PATCH libnbd 1/2] info: Avoid calling nbd_opt_abort if not in option negotiation mode

These patches aren't related to each other, but both are quite simple.

The second one requires particular attention - it's my experience that
printing out the state transitions in debug mode has never helped me
to diagnose a bug, but it has made the debug logs huge and hard to
follow.  However that might just be me!  Has it helped anyone else?
Also I'm open to the concept of debug levels (LIBNBD_DEBUG=2 etc)

Rich.

This change avoids the following harmless but annoying warning on the
normal exit path:

libnbd: debug: nbd1: nbd_opt_abort: enter:
libnbd: debug: nbd1: nbd_opt_abort: leave: error="nbd_opt_abort: invalid
state: READY: the handle must be negotiating: Invalid argument"
---
 info/main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/info/main.c b/info/main.c
index 1b99e08966..c6b3fca0f8 100644
--- a/info/main.c
+++ b/info/main.c
@@ -136,6 +136,7 @@ main (int argc, char *argv[])
   size_t output_len = 0;
   bool content_flag = false, no_content_flag = false;
   bool list_okay = true;
+  bool opt_mode = false;
 
   progname = argv[0];
   colour = isatty (STDOUT_FILENO);
@@ -278,7 +279,8 @@ main (int argc, char *argv[])
   nbd_set_uri_allow_local_file (nbd, true); /* Allow ?tls-psk-file. */
 
   /* Set optional modes in the handle. */
-  if (!can && !map && !size_only) {
+  opt_mode = !can && !map && !size_only;
+  if (opt_mode) {
     nbd_set_opt_mode (nbd, true);
     nbd_set_full_info (nbd, true);
   }
@@ -345,7 +347,8 @@ main (int argc, char *argv[])
   }
 
   free_exports ();
-  nbd_opt_abort (nbd);
+  if (opt_mode)
+    nbd_opt_abort (nbd);
   nbd_shutdown (nbd, 0);
   nbd_close (nbd);
 
-- 
2.40.1

Logging state transitions in debug mode produces huge amounts of
output which is not especially helpful.  This change removes this
debugging output.  This reduces the debug output by approximately two
thirds.
---
 generator/state_machine_generator.ml | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/generator/state_machine_generator.ml
b/generator/state_machine_generator.ml
index 274e290952..52c497646d 100644
--- a/generator/state_machine_generator.ml
+++ b/generator/state_machine_generator.ml
@@ -380,9 +380,6 @@ let
       pr "        h, &next, blocked\n";
       pr "      );\n";
       pr "  if (get_next_state (h) != next) {\n";
-      pr "    debug (h, \"transition: %%s -> %%s\",\n";
-      pr "           \"%s\",\n" display_name;
-      pr "           nbd_internal_state_short_string (next));\n";
       pr "    set_next_state (h, next);\n";
       pr "  }\n";
       pr "  return r;\n";
@@ -423,15 +420,6 @@ let
             pr "    case %s:\n" (c_string_of_external_event e);
             if state != next_state then (
               pr "      set_next_state (h, %s);\n"
next_state.parsed.state_enum;
-              pr "      debug (";
-              let print_debug_args () -                pr "h, \"event
%%s: %%s -> %%s\", ";
-                pr "\"%s\", \"%s\",
\"%s\");"
-                   (string_of_external_event e)
-                   display_name next_state.parsed.display_name;
-              in
-              pr_wrap ',' print_debug_args;
-              pr "\n"
             );
             pr "      goto ok;\n";
         ) events;
-- 
2.40.1

On Mon, Jun 12, 2023 at 07:27:51PM +0100, Richard W.M. Jones
wrote:
> These patches aren't related to each other, but both are quite simple.
> 
> The second one requires particular attention - it's my experience that
> printing out the state transitions in debug mode has never helped me
> to diagnose a bug, but it has made the debug logs huge and hard to
> follow.  However that might just be me!  Has it helped anyone else?
> Also I'm open to the concept of debug levels (LIBNBD_DEBUG=2 etc)
When adding a state, I've found it mildly helpful; but that's not the
common scenario.  I agree that it is too verbose for normal usage.
It's easy enough to revert this patch, or to amend the patch to output
#if 0 or some other witness macro to compile with
CFLAGS=-DLIBNBD_STATE_VERBOSE=1, or similar, if we think making this
easy to re-enable is important.  Otherwise, I'm fine with cutting it
out.

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