Libguestfs - Nov 2022 - [libnbd PATCH v2 2/3] nbdsh: Allow -u interleaved with -c

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

On Fri, Nov 4, 2022 at 11:18 PM Eric Blake <eblake at redhat.com> wrote
[...]

Sorry but I did not read, but I noticed this:

> @@ -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:
>
If False will never run, so why not remove the entire branch?

Is this leftover from debugging?

Nir
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://listman.redhat.com/archives/libguestfs/attachments/20221105/b3dd4ad1/attachment.htm>