Libguestfs - Nov 2022 - [libnbd PATCH v2 0/3] Improve nbdsh -u handling

v1 was here:
https://listman.redhat.com/archives/libguestfs/2022-October/030216.html

Since then, I've incorporated changes based on Rich's feedback:
 swap order of patches 2 and 3
 less change in patch 1 (including no unsafe eval(%s) for --uri)
 in patch 2, include -c in list of snippets to store, and use dict of
   lambdas to map back to the desired action

Eric Blake (3):
  nbdsh: Refactor handling of -c
  nbdsh: Allow -u interleaved with -c
  nbdsh: Improve --help and initial banner contents.

 python/nbdsh.py    | 142 +++++++++++++++++++++++++++------------------
 sh/test-context.sh |  26 ++++-----
 sh/test-error.sh   |  37 +++++++-----
 3 files changed, 119 insertions(+), 86 deletions(-)

-- 
2.38.1

By telling argparse to always supply a list for args.command, it
becomes easier to sink all logic related to going interactive to after
all command line options have been processed.  This in turn will make
future patches that preserve command line ordering easier to manage.
---
 python/nbdsh.py | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/python/nbdsh.py b/python/nbdsh.py
index a98a9113..1552ac73 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -1,5 +1,5 @@
 # NBD client library in userspace
-# Copyright (C) 2013-2021 Red Hat Inc.
+# Copyright (C) 2013-2022 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
@@ -33,6 +33,8 @@ def shell():
     epilog = '''Please read the nbdsh(1) manual page for full
usage.'''
     parser = argparse.ArgumentParser(prog='nbdsh',
description=description,
                                      epilog=epilog)
+
+    parser.set_defaults(command=[])
     short_options = []
     long_options = []

@@ -98,10 +100,6 @@ def shell():
         print("\n".join(long_options))
         exit(0)

-    # Create the banner and prompt.
-    banner = make_banner(args)
-    sys.ps1 = "nbd> "
-
     # If verbose, set LIBNBD_DEBUG=1
     if args.verbose:
         os.environ["LIBNBD_DEBUG"] = "1"
@@ -126,26 +124,27 @@ def shell():
                       (args.uri, ex.string), file=sys.stderr)
                 sys.exit(1)

-    # If there are no -c or --command parameters, go interactive,
-    # otherwise we run the commands and exit.
-    if not args.command:
-        code.interact(banner=banner, local=locals(), exitmsg='')
-    else:
-        # https://stackoverflow.com/a/11754346
-        d = dict(locals(), **globals())
-        try:
-            for c in args.command:
-                if c != '-':
-                    exec(c, d, d)
-                else:
-                    exec(sys.stdin.read(), d, d)
-        except nbd.Error as ex:
-            if nbd.NBD().get_debug():
-                traceback.print_exc()
+    # Run all -c snippets
+    # https://stackoverflow.com/a/11754346
+    d = dict(locals(), **globals())
+    try:
+        for c in args.command:
+            if c != '-':
+                exec(c, d, d)
             else:
-                print("nbdsh: command line script failed: %s" %
ex.string,
-                      file=sys.stderr)
-            sys.exit(1)
+                exec(sys.stdin.read(), d, d)
+    except nbd.Error as ex:
+        if nbd.NBD().get_debug():
+            traceback.print_exc()
+        else:
+            print("nbdsh: command line script failed: %s" %
ex.string,
+                  file=sys.stderr)
+        sys.exit(1)
+
+    # If there are no explicit -c or --command parameters, go interactive.
+    if len(args.command) == 0:
+        sys.ps1 = "nbd> "
+        code.interact(banner=make_banner(args), local=locals(),
exitmsg='')


 def make_banner(args):
-- 
2.38.1

Starting with a back-story: I wanted[1] to write:

$ nbdkit --tls=require --tls-psk=$dir/tests/keys.psk null --run \
 'nbdsh -u "nbds://alice at
localhost/?tls-psk-file='"$dir"'/tests/keys.psk" \
   -c "h.get_size()"'

but it fails with the latest libnbd release, due to:

nbdsh: unable to connect to uri 'nbds://alice at
localhost/?tls-psk-file=/home/eblake/libnbd/tests/keys.psk':
nbd_connect_uri: local file access (tls-psk-file) is not allowed, call
nbd_set_uri_allow_local_file to enable this: Operation not permitted

But without this patch, this obvious attempt at a fix doesn't work:

$ nbdkit --tls=require --tls-psk=$dir/tests/keys.psk null --run \
 'nbdsh -c "h.set_uri_allow_local_file(True)" \
   -u "nbds://alice at
localhost/?tls-psk-file='"$dir"'/tests/keys.psk" \
   -c "h.get_size()"'

because nbdsh was performing h.connect_uri() prior to running any of
the -c strings, where the attempt to inject -c to match the hint from
the original error message is processed too late, even though it
occurred earlier in the command line.  The immediate workaround is
thus to spell out the URI with a manual -c:

$ nbdkit --tls=require --tls-psk=$dir/tests/keys.psk null --run \
 'nbdsh -c "h.set_uri_allow_local_file(True)" \
   -c "h.connect_uri(\"nbds://alice at
localhost/?tls-psk-file='"$dir"'/tests/keys.psk\")"
\
   -c "h.get_size()"'

which gets awkward because of the multiple layers of quoting required
(--run, -c, and Python each strip a layer).  But we can do better.
Instead of creating separate args.command and args.uri, we can use a
custom argparse Action subclass which will merge various command-line
options into a unified args.snippets.  Then with a little bit of
lambda magic, dispatching the various snippets in order looks fairly
easy.

I did have to drop one use of args.uri in the help banner, but it will
be restored in improved form in the next patch.

test-error.sh and test-context.sh have to be updated to match our
saner processing order.  nbdkit's testsuite still passes even when
using this update to nbdsh, so I don't think programs in the wild were
relying on insane command-line rearranging.

[1] Actually, I wanted to use -U- and write -u "$uri" instead of -u
"nbds+unix...", but that involves teaching nbdkit how to display a
more-useful URI when TLS is in use.  Not this patch's problem.
---
 python/nbdsh.py    | 76 +++++++++++++++++++++++++++-------------------
 sh/test-context.sh | 26 ++++++++--------
 sh/test-error.sh   | 37 ++++++++++++----------
 3 files changed, 77 insertions(+), 62 deletions(-)

diff --git a/python/nbdsh.py b/python/nbdsh.py
index 1552ac73..0919c9ec 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -34,15 +34,37 @@ def shell():
     parser = argparse.ArgumentParser(prog='nbdsh',
description=description,
                                      epilog=epilog)

-    parser.set_defaults(command=[])
+    # Allow intermixing of various options for replay in command-line order:
+    # each option registered with this Action subclass will append a tuple
+    # to a single list of snippets
+    class SnippetAction(argparse.Action):
+        def __init__(self, option_strings, dest, nargs=None,
+                     default=argparse.SUPPRESS, **kwargs):
+            if nargs not in [0, None]:
+                raise ValueError("nargs must be 0 or None")
+            super().__init__(option_strings, dest, nargs=nargs,
+                             default=default, **kwargs)
+
+        def __call__(self, parser, namespace, values, option_string=None):
+            dest = self.dest
+            if dest != 'command':
+                setattr(namespace, 'need_handle',
+                        getattr(namespace, 'need_handle') + 1)
+            elif values == '-':
+                dest = 'stdin'
+            snippets = getattr(namespace, 'snippets')[:]
+            snippets.append((dest, values))
+            setattr(namespace, 'snippets', snippets)
+
+    parser.set_defaults(need_handle=0, snippets=[])
     short_options = []
     long_options = []

-    parser.add_argument('--base-allocation',
action='store_true',
+    parser.add_argument('--base-allocation', action=SnippetAction,
nargs=0,
                         help='request the "base:allocation" meta
context')
     long_options.append("--base-allocation")

-    parser.add_argument('-c', '--command',
action='append',
+    parser.add_argument('-c', '--command',
action=SnippetAction,
                         help="run a Python statement "
                         "(may be used multiple times)")
     short_options.append("-c")
@@ -52,15 +74,17 @@ def shell():
                         help="do not create the implicit handle
'h'")
     short_options.append("-n")

-    parser.add_argument('--opt-mode', action='store_true',
+    parser.add_argument('--opt-mode', action=SnippetAction, nargs=0,
                         help='request opt mode during connection')
     long_options.append("--opt-mode")

-    parser.add_argument('-u', '--uri', help="connect to
NBD URI")
+    parser.add_argument('-u', '--uri', action=SnippetAction,
+                        help="connect to NBD URI")
     short_options.append("-u")
     long_options.append("--uri")
     # For back-compat, provide --connect as an undocumented synonym to --uri
-    parser.add_argument('--connect', dest='uri',
help=argparse.SUPPRESS)
+    parser.add_argument('--connect', dest='uri',
action=SnippetAction,
+                        help=argparse.SUPPRESS)

     parser.add_argument('-v', '--verbose',
action='store_true')
     short_options.append("-v")
@@ -79,9 +103,7 @@ def shell():
     args = parser.parse_args()

     # It's an error if -n is passed with certain other options.
-    if args.n and (args.base_allocation or
-                   args.opt_mode or
-                   args.uri is not None):
+    if args.n and args.need_handle:
         print("error: -n option cannot be used with " +
               "--base-allocation, --opt-mode or --uri",
               file=sys.stderr)
@@ -109,30 +131,20 @@ def shell():
         h = nbd.NBD()
         h.set_handle_name("nbdsh")

-        # Set other attributes in the handle.
-        if args.base_allocation:
-            h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)
-        if args.opt_mode:
-            h.set_opt_mode(True)
-
-        # Parse the URI.
-        if args.uri is not None:
-            try:
-                h.connect_uri(args.uri)
-            except nbd.Error as ex:
-                print("nbdsh: unable to connect to uri '%s':
%s" %
-                      (args.uri, ex.string), file=sys.stderr)
-                sys.exit(1)
-
-    # Run all -c snippets
+    # Run all snippets
     # https://stackoverflow.com/a/11754346
     d = dict(locals(), **globals())
+    do_snippet = {
+        "command": lambda arg: exec(arg, d, d),
+        "stdin": lambda arg: exec(sys.stdin.read(), d, d),
+        "base_allocation": lambda arg: h.add_meta_context(
+            nbd.CONTEXT_BASE_ALLOCATION),
+        "opt_mode": lambda arg: h.set_opt_mode(True),
+        "uri": lambda arg: h.connect_uri(arg),
+    }
     try:
-        for c in args.command:
-            if c != '-':
-                exec(c, d, d)
-            else:
-                exec(sys.stdin.read(), d, d)
+        for (act, arg) in args.snippets:
+            do_snippet[act](arg)
     except nbd.Error as ex:
         if nbd.NBD().get_debug():
             traceback.print_exc()
@@ -142,7 +154,7 @@ def shell():
         sys.exit(1)

     # If there are no explicit -c or --command parameters, go interactive.
-    if len(args.command) == 0:
+    if len(args.snippets) - args.need_handle == 0:
         sys.ps1 = "nbd> "
         code.interact(banner=make_banner(args), local=locals(),
exitmsg='')

@@ -165,7 +177,7 @@ def example(ex, desc): line("%-34s # %s" % (ex,
desc))
         line("The ???nbd??? module has already been imported.")
         blank()
         example("h = nbd.NBD()", "Create a new handle.")
-    if args.uri is None:
+    if False:  # args.uri is None:
         example('h.connect_tcp("remote", "10809")',
                 "Connect to a remote server.")
     example("h.get_size()", "Get size of the remote disk.")
diff --git a/sh/test-context.sh b/sh/test-context.sh
index 168fe487..8f759075 100755
--- a/sh/test-context.sh
+++ b/sh/test-context.sh
@@ -31,15 +31,13 @@ if test "x$output" != xFalse; then
     fail=1
 fi

-# Use of -c to request context is too late with -u
-output=$(nbdkit -U - null --run 'nbdsh -c "
-try:
-    h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)
-    assert False
-except nbd.Error:
-    print(\"okay\")
-" -u "nbd+unix://?socket=$unixsocket"')
-if test "x$output" != xokay; then
+# Can also use manual -c to request context before -u
+output=$(nbdkit -U - null --run 'nbdsh \
+-c "h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)" \
+-u "nbd+unix://?socket=$unixsocket" \
+-c "print(h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION))"
+')
+if test "x$output" != xTrue; then
     echo "$0: unexpected output: $output"
     fail=1
 fi
@@ -53,9 +51,9 @@ if test "x$output" != xTrue; then
     fail=1
 fi

-# Again, but with --b after -u, and with abbreviated option names
+# Again, but with abbreviated option names
 output=$(nbdkit -U - null --run 'nbdsh \
-    -u "nbd+unix://?socket=$unixsocket" --b \
+    --b -u "nbd+unix://?socket=$unixsocket" \
     -c "print(h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION))"')
 if test "x$output" != xTrue; then
     echo "$0: unexpected output: $output"
@@ -65,7 +63,7 @@ fi
 if [[ $(nbdkit --help) =~ --no-sr ]]; then
     # meta context depends on server cooperation
     output=$(nbdkit -U - --no-sr null --run 'nbdsh \
-      -u "nbd+unix://?socket=$unixsocket" --base-allocation \
+      --base-allocation -u "nbd+unix://?socket=$unixsocket" \
       -c
"print(h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION))"')
     if test "x$output" != xFalse; then
         echo "$0: unexpected output: $output"
@@ -77,7 +75,7 @@ fi

 # Test interaction with opt mode
 output=$(nbdkit -U - null --run 'nbdsh \
-    -u "nbd+unix://?socket=$unixsocket" --opt-mode --base-allocation
\
+    --opt-mode --base-allocation -u "nbd+unix://?socket=$unixsocket"
\
     -c "
 try:
     h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION)
@@ -94,7 +92,7 @@ fi

 # And with --opt-mode, we can get away without --base-allocation
 output=$(nbdkit -U - null --run 'nbdsh \
-    -u "nbd+unix://?socket=$unixsocket" --opt-mode \
+    --opt-mode -u "nbd+unix://?socket=$unixsocket" \
     -c "h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)" \
     -c "h.opt_go()" \
     -c "print(h.can_meta_context(nbd.CONTEXT_BASE_ALLOCATION))"')
diff --git a/sh/test-error.sh b/sh/test-error.sh
index a33ce475..7dbd7ae4 100755
--- a/sh/test-error.sh
+++ b/sh/test-error.sh
@@ -35,32 +35,37 @@ cat $err
 grep Traceback $err && fail=1
 grep '^nbdsh: .*unrecognized.*no-such' $err

-# Test behavior when -u fails.  No python trace should be present.
-nbdsh -u 'nbd+unix:///?socket=/nosuchsock' >$out 2>$err
&& fail=1
-test ! -s $out
-cat $err
-grep Traceback $err && fail=1
-grep '^nbdsh: unable to connect to uri.*nosuchsock' $err
-
 # Triggering nbd.Error non-interactively (via -c) prints the error. The
-# output includes the python trace when debugging is enabled (which is
-# the default for our testsuite, when using ./run).
-nbdsh -c 'h.is_read_only()' >$out 2>$err && fail=1
+# output includes the python trace when debugging is enabled (our default
+# when run under 'make check', but set explicitly here to make sure).
+LIBNBD_DEBUG=1 nbdsh -c 'h.is_read_only()' >$out 2>$err
&& fail=1
 test ! -s $out
 cat $err
 grep Traceback $err
 grep 'in is_read_only' $err
 grep '^nbd\.Error: nbd_is_read_only: ' $err

-# Override ./run's default to show that without debug, the error is
succinct.
-nbdsh -c '
-import os
-os.environ["LIBNBD_DEBUG"] = "0"
-h.is_read_only()
-' >$out 2>$err && fail=1
+# Without debugging, the error is succinct.
+LIBNBD_DEBUG=0 nbdsh -c 'h.is_read_only()' >$out 2>$err
&& fail=1
 test ! -s $out
 cat $err
 grep Traceback $err && fail=1
 grep '^nbdsh: command line script failed: nbd_is_read_only: ' $err

+# --verbose overrides environment to request debugging.
+LIBNBD_DEBUG=0 nbdsh --verbose -c 'h.is_read_only()' >$out 2>$err
&& fail=1
+test ! -s $out
+cat $err
+grep Traceback $err
+grep 'in is_read_only' $err
+grep '^nbd\.Error: nbd_is_read_only: ' $err
+
+# Test behavior when -u fails; since it triggers nbd.Error, it should
+# be succinct without debug.
+LIBNBD_DEBUG=0 nbdsh -u 'nbd+unix:///?socket=/nosuchsock' >$out
2>$err && fail=1
+test ! -s $out
+cat $err
+grep Traceback $err && fail=1
+grep '^nbdsh: .*nbd_connect_uri: ' $err
+
 exit $fail
-- 
2.38.1

Document all options in --help output.  If -n is not in use, then
enhance the banner to print the current state of h, and further tailor
the advice given on useful next steps to take to mention opt_go when
using --opt-mode.
---
 python/nbdsh.py | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/python/nbdsh.py b/python/nbdsh.py
index 0919c9ec..872b94fc 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -86,11 +86,13 @@ def __call__(self, parser, namespace, values,
option_string=None):
     parser.add_argument('--connect', dest='uri',
action=SnippetAction,
                         help=argparse.SUPPRESS)

-    parser.add_argument('-v', '--verbose',
action='store_true')
+    parser.add_argument('-v', '--verbose',
action='store_true',
+                        help="enable verbose debugging")
     short_options.append("-v")
     long_options.append("--verbose")

-    parser.add_argument('-V', '--version',
action='store_true')
+    parser.add_argument('-V', '--version',
action='store_true',
+                        help="display version information")
     short_options.append("-V")
     long_options.append("--version")

@@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
option_string=None):
         os.environ["LIBNBD_DEBUG"] = "1"

     # Create the handle.
-    if not args.n:
+    if args.n:
+        pass
+    else:
+        global h
         h = nbd.NBD()
         h.set_handle_name("nbdsh")

@@ -165,21 +170,35 @@ def line(x): lines.append(x)
     def blank(): line("")
     def example(ex, desc): line("%-34s # %s" % (ex, desc))

+    connect_hint = False
+    go_hint = False
     blank()
     line("Welcome to nbdsh, the shell for interacting with")
     line("Network Block Device (NBD) servers.")
     blank()
-    if not args.n:
-        line("The ???nbd??? module has already been imported and
there")
-        line("is an open NBD handle called ???h???.")
-        blank()
-    else:
+    if args.n:
         line("The ???nbd??? module has already been imported.")
         blank()
         example("h = nbd.NBD()", "Create a new handle.")
-    if False:  # args.uri is None:
+        connect_hint = True
+    else:
+        global h
+        state = h.connection_state()
+        state = state[:state.find(':')]
+        line("The ???nbd??? module has already been imported and
there")
+        line("is an open NBD handle called ???h??? in state
'%s'." % state)
+        blank()
+        if h.aio_is_created():
+            connect_hint = True
+            if h.get_opt_mode():
+                go_hint = True
+        elif h.aio_is_negotiating():
+            go_hint = True
+    if connect_hint:
         example('h.connect_tcp("remote", "10809")',
                 "Connect to a remote server.")
+    if go_hint:
+        example("h.opt_go()", "Finish option negotiation")
     example("h.get_size()", "Get size of the remote disk.")
     example("buf = h.pread(512, 0)", "Read the first
sector.")
     example("exit() or Ctrl-D", "Quit the shell")
-- 
2.38.1