[Libguestfs] [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@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@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@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@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