Eric Blake
2022-Oct-26 21:16 UTC
[Libguestfs] [libnbd PATCH 3/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 (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
Richard W.M. Jones
2022-Oct-27 08:37 UTC
[Libguestfs] [libnbd PATCH 3/3] nbdsh: Allow -u interleaved with -c
It's confusing having patches 1 & 3 separate. I can't help thinking this whole series would be easier to understand if instead of building a list of Python commands as strings, we built a list of "instructions", where an instruction is a qualified union which (in OCaml) would be: type instruction | Command of string (* -c *) | URI of string (* -u *) | OptMode (* --opt-mode *) etc and then we'd run through the instructions in order at the end. It gets rid of the awkward quoting problems too. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v