Libguestfs - Oct 2022 - [libnbd PATCH 1/3] nbdsh: Refactor handling of -u and friends

I was trying to add a TLS test to nbdkit, and got annoyed that I
couldn't just write --run 'nbdsh -u "$uri" -c ...'.  This
fixes the
libnbd side of the problem (a URI with ?tls-psk-file=... is rejected
unless we tell libnbd that parsing local files is okay); but I will
also be working on patches to nbdkit ($uri is more useful if it
includes 'alice@' and '?tls-psk-file=...' in the first place).

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

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

-- 
2.37.3

Slightly rearrange the code so that we can reuse the -c framework for
executing the code snippets for -u, --opt-mode, and --base-allocation.
Slightly changes the error message when a URI is invalid (which
requires revamping test-error.sh a bit to match), but otherwise no
semantic change intended.
---
 python/nbdsh.py  | 67 ++++++++++++++++++++++--------------------------
 sh/test-error.sh | 37 ++++++++++++++------------
 2 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/python/nbdsh.py b/python/nbdsh.py
index a98a9113..84ac84e2 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
@@ -35,12 +35,13 @@ def shell():
                                      epilog=epilog)
     short_options = []
     long_options = []
+    shortcuts = 0

     parser.add_argument('--base-allocation',
action='store_true',
                         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='append', default=[],
                         help="run a Python statement "
                         "(may be used multiple times)")
     short_options.append("-c")
@@ -98,10 +99,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"
@@ -110,42 +107,40 @@ def shell():
     if not args.n:
         h = nbd.NBD()
         h.set_handle_name("nbdsh")
+        cmds = args.command

         # Set other attributes in the handle.
+        if args.uri is not None:
+            cmds.insert(0, 'h.connect_uri("%s")' % args.uri)
+            shortcuts += 1
         if args.base_allocation:
-            h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)
+            cmds.insert(0,
'h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)')
+            shortcuts += 1
         if args.opt_mode:
-            h.set_opt_mode(True)
+            cmds.insert(0, 'h.set_opt_mode(True)')
+            shortcuts += 1

-        # 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)
-
-    # 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, including any shortcuts we just added
+    # 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) - shortcuts == 0:
+        sys.ps1 = "nbd> "
+        code.interact(banner=make_banner(args), local=locals(),
exitmsg='')


 def make_banner(args):
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.37.3

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, 27 insertions(+), 10 deletions(-)

diff --git a/python/nbdsh.py b/python/nbdsh.py
index 84ac84e2..f23d8bfb 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -61,11 +61,13 @@ def shell():
     # For back-compat, provide --connect as an undocumented synonym to --uri
     parser.add_argument('--connect', dest='uri',
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")

@@ -140,30 +142,45 @@ def shell():
     # If there are no explicit -c or --command parameters, go interactive.
     if len(args.command) - shortcuts == 0:
         sys.ps1 = "nbd> "
-        code.interact(banner=make_banner(args), local=locals(),
exitmsg='')
+        if args.n:
+            h = None
+        code.interact(banner=make_banner(args, h), local=locals(),
exitmsg='')


-def make_banner(args):
+def make_banner(args, h):
     lines = []
     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 args.uri is None:
+        connect_hint = True
+    else:
+        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.37.3

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 (seen more obviously two patches ago when refactoring
shortcuts to reuse the -c framework), where the attempt to inject -c
to obey the error message is processed too late, even though it
occurred earlier in the command line.  The immediate workaround is
thus:

$ 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.  We
can create args.command in command-line order by introducing the use
of a custom Action subclass of argparse.  test-context.sh has 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    | 60 ++++++++++++++++++++++++++++++----------------
 sh/test-context.sh | 26 ++++++++++----------
 2 files changed, 51 insertions(+), 35 deletions(-)

diff --git a/python/nbdsh.py b/python/nbdsh.py
index f23d8bfb..370f3fd2 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -33,11 +33,37 @@ def shell():
     epilog = '''Please read the nbdsh(1) manual page for full
usage.'''
     parser = argparse.ArgumentParser(prog='nbdsh',
description=description,
                                      epilog=epilog)
+
+    # We want to treat some options as shortcuts for longer '-c code'
+    # snippets while preserving relative ordering of the overall command
+    # line; use of the shortcuts does not prevent interactive mode.
+    class ShortcutAction(argparse.Action):
+        def __init__(self, option_strings, dest, nargs=None, const=None,
+                     default=argparse.SUPPRESS, **kwargs):
+            if nargs not in [0, 1]:
+                raise ValueError("nargs must be 0 or 1")
+            if const is None:
+                raise ValueError("missing const string for use as
shortcut")
+            super().__init__(option_strings, dest, nargs=nargs, const=const,
+                             default=default, **kwargs)
+
+        def __call__(self, parser, namespace, values, option_string=None):
+            setattr(namespace, 'shortcuts',
+                    getattr(namespace, 'shortcuts') + 1)
+            commands = getattr(namespace, 'command')[:]
+            if self.nargs == 0:
+                commands.append(self.const)
+            else:
+                commands.append(self.const % values[0])
+            setattr(namespace, 'command', commands)
+
+    parser.set_defaults(shortcuts=0)
     short_options = []
     long_options = []
-    shortcuts = 0

-    parser.add_argument('--base-allocation',
action='store_true',
+    parser.add_argument('--base-allocation', action=ShortcutAction,
nargs=0,
+                        const='h.add_meta_context' +
+                        '(nbd.CONTEXT_BASE_ALLOCATION)',
                         help='request the "base:allocation" meta
context')
     long_options.append("--base-allocation")

@@ -51,15 +77,20 @@ 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=ShortcutAction, nargs=0,
+                        const='h.set_opt_mode(True)',
                         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=ShortcutAction,
nargs=1,
+                        const='h.connect_uri("%s")',
+                        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', action=ShortcutAction, nargs=1,
+                        const='h.connect_uri("%s")',
+                        dest='uri', help=argparse.SUPPRESS)

     parser.add_argument('-v', '--verbose',
action='store_true',
                         help="enable verbose debugging")
@@ -80,9 +111,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.shortcuts > 0:
         print("error: -n option cannot be used with " +
               "--base-allocation, --opt-mode or --uri",
               file=sys.stderr)
@@ -111,18 +140,7 @@ def shell():
         h.set_handle_name("nbdsh")
         cmds = args.command

-        # Set other attributes in the handle.
-        if args.uri is not None:
-            cmds.insert(0, 'h.connect_uri("%s")' % args.uri)
-            shortcuts += 1
-        if args.base_allocation:
-            cmds.insert(0,
'h.add_meta_context(nbd.CONTEXT_BASE_ALLOCATION)')
-            shortcuts += 1
-        if args.opt_mode:
-            cmds.insert(0, 'h.set_opt_mode(True)')
-            shortcuts += 1
-
-    # Run all -c snippets, including any shortcuts we just added
+    # Run all -c snippets, including shortcuts
     # https://stackoverflow.com/a/11754346
     d = dict(locals(), **globals())
     try:
@@ -140,7 +158,7 @@ def shell():
         sys.exit(1)

     # If there are no explicit -c or --command parameters, go interactive.
-    if len(args.command) - shortcuts == 0:
+    if len(args.command) - args.shortcuts == 0:
         sys.ps1 = "nbd> "
         if args.n:
             h = None
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))"')
-- 
2.37.3