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

On Thu, Oct 27, 2022 at 09:28:19AM +0100, Richard W.M. Jones wrote: Good observation. The old way... ...directly passed the caller's string as a single unit, instead of... ...possibly splitting it through exec(). Yes, this part of the code motion is because sys.ps1 only matters prior to code.interact(). Your reply to 3/3 has merit; I'll add more there. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

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 (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@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