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

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 Wed, Oct 26, 2022 at 04:15:59PM -0500, Eric Blake
wrote:
> 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)
This allows commands to be injected if, for example, the nbdsh -u
parameter came from an untrusted source.

After going through probably hundreds of stackoverflow answer this
morning I cannot find if Python has an official way to escape this.
Both string.encode() and %r seem like potential candidates:

  >>> s = "\""
  >>> 'h.connect_uri(%s)' % s.encode('unicode-escape')
  'h.connect_uri(b\'"\')'
  >>> 'h.connect_uri(%r)' % s
  'h.connect_uri(\'"\')'

(Note the quoting displayed there is a little confusing because the
output has been quoted.)
> +            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='')
Previously we were unconditionally setting sys.ps1.  Don't know if the
new behaviour is correct - possibly it is because we will only use
sys.ps1 when we call code.interact.

Rich.
> 
>  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
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW