4:18 p.m.
v1 was here:
https://listman.redhat.com/archives/libguestfs/2022-October/030216.html
Since then, I've incorporated changes based on Rich's feedback:
swap order of patches 2 and 3
less change in patch 1 (including no unsafe eval(%s) for --uri)
in patch 2, include -c in list of snippets to store, and use dict of
lambdas to map back to the desired action
Eric Blake (3):
nbdsh: Refactor handling of -c
nbdsh: Allow -u interleaved with -c
nbdsh: Improve --help and initial banner contents.
python/nbdsh.py | 142 +++++++++++++++++++++++++++------------------
sh/test-context.sh | 26 ++++-----
sh/test-error.sh | 37 +++++++-----
3 files changed, 119 insertions(+), 86 deletions(-)
--
2.38.1
4:18 p.m.
New subject: [libnbd PATCH v2 1/3] nbdsh: Refactor handling of -c
By telling argparse to always supply a list for args.command, it
becomes easier to sink all logic related to going interactive to after
all command line options have been processed. This in turn will make
future patches that preserve command line ordering easier to manage.
---
python/nbdsh.py | 47 +++++++++++++++++++++++------------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/python/nbdsh.py b/python/nbdsh.py
index a98a9113..1552ac73 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
@@ -33,6 +33,8 @@ def shell():
epilog = '''Please read the nbdsh(1) manual page for full
usage.'''
parser = argparse.ArgumentParser(prog='nbdsh', description=description,
epilog=epilog)
+
+ parser.set_defaults(command=[])
short_options = []
long_options = []
@@ -98,10 +100,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"
@@ -126,26 +124,27 @@ def shell():
(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
+ # 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) == 0:
+ sys.ps1 = "nbd> "
+ code.interact(banner=make_banner(args), local=locals(), exitmsg='')
def make_banner(args):
--
2.38.1
4:18 p.m.
New subject: [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
8:32 p.m.
New subject: [libnbd PATCH v2 2/3] nbdsh: Allow -u interleaved with -c
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake <eblake(a)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
1:29 p.m.
New subject: [libnbd PATCH v2 2/3] nbdsh: Allow -u interleaved with -c
On Sat, Nov 05, 2022 at 03:32:04AM +0200, Nir Soffer wrote:
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake <eblake(a)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?
This patch gets rid of args.uri, but the next patch tweaks the banner
to output the same message based on whether h is already connected. I
can wipe the dead code in this patch, but the example() on the next
line would then re-appear in the next patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
4:18 p.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
Document all options in --help output. If -n is not in use, then
enhance the banner to print the current state of h, and further tailor
the advice given on useful next steps to take to mention opt_go when
using --opt-mode.
---
python/nbdsh.py | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/python/nbdsh.py b/python/nbdsh.py
index 0919c9ec..872b94fc 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -86,11 +86,13 @@ def __call__(self, parser, namespace, values, option_string=None):
parser.add_argument('--connect', dest='uri', action=SnippetAction,
help=argparse.SUPPRESS)
- parser.add_argument('-v', '--verbose', action='store_true')
+ parser.add_argument('-v', '--verbose', action='store_true',
+ help="enable verbose debugging")
short_options.append("-v")
long_options.append("--verbose")
- parser.add_argument('-V', '--version', action='store_true')
+ parser.add_argument('-V', '--version', action='store_true',
+ help="display version information")
short_options.append("-V")
long_options.append("--version")
@@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values, option_string=None):
os.environ["LIBNBD_DEBUG"] = "1"
# Create the handle.
- if not args.n:
+ if args.n:
+ pass
+ else:
+ global h
h = nbd.NBD()
h.set_handle_name("nbdsh")
@@ -165,21 +170,35 @@ def line(x): lines.append(x)
def blank(): line("")
def example(ex, desc): line("%-34s # %s" % (ex, desc))
+ connect_hint = False
+ go_hint = False
blank()
line("Welcome to nbdsh, the shell for interacting with")
line("Network Block Device (NBD) servers.")
blank()
- if not args.n:
- line("The ‘nbd’ module has already been imported and there")
- line("is an open NBD handle called ‘h’.")
- blank()
- else:
+ if args.n:
line("The ‘nbd’ module has already been imported.")
blank()
example("h = nbd.NBD()", "Create a new handle.")
- if False: # args.uri is None:
+ connect_hint = True
+ else:
+ global h
+ state = h.connection_state()
+ state = state[:state.find(':')]
+ line("The ‘nbd’ module has already been imported and there")
+ line("is an open NBD handle called ‘h’ in state '%s'." %
state)
+ blank()
+ if h.aio_is_created():
+ connect_hint = True
+ if h.get_opt_mode():
+ go_hint = True
+ elif h.aio_is_negotiating():
+ go_hint = True
+ if connect_hint:
example('h.connect_tcp("remote", "10809")',
"Connect to a remote server.")
+ if go_hint:
+ example("h.opt_go()", "Finish option negotiation")
example("h.get_size()", "Get size of the remote disk.")
example("buf = h.pread(512, 0)", "Read the first sector.")
example("exit() or Ctrl-D", "Quit the shell")
--
2.38.1
8:39 p.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake <eblake(a)redhat.com> wrote:
[...]
@@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
option_string=None):
os.environ["LIBNBD_DEBUG"] = "1"
# Create the handle.
- if not args.n:
+ if args.n:
+ pass
+ else:
Why add useless branch?
+ global h
h = nbd.NBD()
h.set_handle_name("nbdsh")
@@ -165,21 +170,35 @@ def line(x): lines.append(x)
def blank(): line("")
def example(ex, desc): line("%-34s # %s" % (ex, desc))
+ connect_hint = False
+ go_hint = False
blank()
line("Welcome to nbdsh, the shell for interacting with")
line("Network Block Device (NBD) servers.")
blank()
- if not args.n:
- line("The ‘nbd’ module has already been imported and there")
- line("is an open NBD handle called ‘h’.")
- blank()
- else:
+ if args.n:
line("The ‘nbd’ module has already been imported.")
blank()
example("h = nbd.NBD()", "Create a new handle.")
- if False: # args.uri is None:
Good that this was removed, but it will be better to remove in the previous
patch.
Nir
6:24 a.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Sat, Nov 05, 2022 at 03:39:26AM +0200, Nir Soffer wrote:
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake <eblake(a)redhat.com>
wrote:
[...]
@@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
option_string=None):
os.environ["LIBNBD_DEBUG"] = "1"
# Create the handle.
- if not args.n:
+ if args.n:
+ pass
+ else:
Why add useless branch?
Yes this is a bit of a strange change. What was wrong with the original
"if not ..."?
Perl's "unless" statement FTW :-)
+ global h
h = nbd.NBD()
h.set_handle_name("nbdsh")
@@ -165,21 +170,35 @@ def line(x): lines.append(x)
def blank(): line("")
def example(ex, desc): line("%-34s # %s" % (ex, desc))
+ connect_hint = False
+ go_hint = False
blank()
line("Welcome to nbdsh, the shell for interacting with")
line("Network Block Device (NBD) servers.")
blank()
- if not args.n:
- line("The ‘nbd’ module has already been imported and there")
- line("is an open NBD handle called ‘h’.")
- blank()
- else:
+ if args.n:
line("The ‘nbd’ module has already been imported.")
blank()
example("h = nbd.NBD()", "Create a new handle.")
- if False: # args.uri is None:
Good that this was removed, but it will be better to remove in the previous
patch.
I believe for intermediate patches this is OK, if it's resolved in the end.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
1:31 p.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Sat, Nov 05, 2022 at 03:39:26AM +0200, Nir Soffer wrote:
On Fri, Nov 4, 2022 at 11:18 PM Eric Blake <eblake(a)redhat.com>
wrote:
[...]
> @@ -127,7 +129,10 @@ def __call__(self, parser, namespace, values,
> option_string=None):
> os.environ["LIBNBD_DEBUG"] = "1"
>
> # Create the handle.
> - if not args.n:
> + if args.n:
> + pass
> + else:
>
Why add useless branch?
At one point in the evolution of my patch, I tried setting 'h = None'
in the args.n branch, but that broke sh/test-implicit-handle.sh. The
fact that I dropped it back to pass instead of deleting the dead
branch was a poor rebase act on my part; I'll clean it back up to the
original if not args.n branching.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
6:24 a.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
For the series:
Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
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
2:53 p.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Sat, Nov 05, 2022 at 11:24:20AM +0000, Richard W.M. Jones wrote:
For the series:
Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
I cleaned up based on Nir's review, then pushed the series as
49c2375a..61e88a65
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
4:34 p.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
Document all options in --help output. If -n is not in use, then
enhance the banner to print the current state of h, and further tailor
the advice given on useful next steps to take to mention opt_go when
using --opt-mode.
I had to partially revert this patch (reverting most of it) because it
unfortunately breaks the implicit handle creation :-(
https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3...
I'm not actually sure how to do this correctly in Python. I made
several attempts, but I don't think Python is very good about having a
variable which is only defined on some paths -- maybe it's not
possible at all.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
5:33 a.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones <rjones(a)redhat.com> wrote:
On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> Document all options in --help output. If -n is not in use, then
> enhance the banner to print the current state of h, and further tailor
> the advice given on useful next steps to take to mention opt_go when
> using --opt-mode.
I had to partially revert this patch (reverting most of it) because it
unfortunately breaks the implicit handle creation :-(
https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3...
I'm not actually sure how to do this correctly in Python. I made
several attempts, but I don't think Python is very good about having a
variable which is only defined on some paths -- maybe it's not
possible at all.
Can you share the error when it breaks?
I'm not sure what is the issue, but usually if you have a global
variable created only in
some flows, adding:
thing = None
At the start of the module makes sure that the name exists later,
regardless of the flow
taken. Code can take the right action based on:
if thing is None:
...
Nir
5:52 a.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones
<rjones(a)redhat.com> wrote:
>
> On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > Document all options in --help output. If -n is not in use, then
> > enhance the banner to print the current state of h, and further tailor
> > the advice given on useful next steps to take to mention opt_go when
> > using --opt-mode.
>
> I had to partially revert this patch (reverting most of it) because it
> unfortunately breaks the implicit handle creation :-(
>
> https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3...
>
> I'm not actually sure how to do this correctly in Python. I made
> several attempts, but I don't think Python is very good about having a
> variable which is only defined on some paths -- maybe it's not
> possible at all.
Can you share the error when it breaks?
$ rpm -qf /usr/bin/nbdsh
python3-libnbd-1.15.9-2.fc38.x86_64
$ nbdsh
Welcome to nbdsh, the shell for interacting with
Network Block Device (NBD) servers.
The ‘nbd’ module has already been imported and there
is an open NBD handle called ‘h’ in state 'START'.
h.connect_tcp("remote", "10809") # Connect to a remote server.
h.get_size() # Get size of the remote disk.
buf = h.pread(512, 0) # Read the first sector.
exit() or Ctrl-D # Quit the shell
help(nbd) # Display documentation
nbd> print(h)
Traceback (most recent call last):
File "/usr/lib64/python3.11/code.py", line 90, in runcode
exec(code, self.locals)
File "<console>", line 1, in <module>
NameError: name 'h' is not defined
I'm not sure what is the issue, but usually if you have a global
variable created only in
some flows, adding:
thing = None
At the start of the module makes sure that the name exists later,
regardless of the flow
taken. Code can take the right action based on:
if thing is None:
...
Good point, I was trying 'undef h' instead. 'h' not being present in
the dictionary at all vs 'h = None' are slightly different, although I
suppose it doesn't matter in this particular case.
The other point is that 'h' (when defined) is not a global. The patch
assumes it is a global, but then uses it in some places as if it is a
local.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
10:41 a.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Tue, Jan 31, 2023 at 11:52:27AM +0000, Richard W.M. Jones wrote:
On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
> On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones <rjones(a)redhat.com>
wrote:
> >
> > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > > Document all options in --help output. If -n is not in use, then
> > > enhance the banner to print the current state of h, and further tailor
> > > the advice given on useful next steps to take to mention opt_go when
> > > using --opt-mode.
> >
> > I had to partially revert this patch (reverting most of it) because it
> > unfortunately breaks the implicit handle creation :-(
> >
> >
https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3...
> >
> > I'm not actually sure how to do this correctly in Python. I made
> > several attempts, but I don't think Python is very good about having a
> > variable which is only defined on some paths -- maybe it's not
> > possible at all.
>
> Can you share the error when it breaks?
$ rpm -qf /usr/bin/nbdsh
python3-libnbd-1.15.9-2.fc38.x86_64
$ nbdsh
Welcome to nbdsh, the shell for interacting with
Network Block Device (NBD) servers.
The ‘nbd’ module has already been imported and there
is an open NBD handle called ‘h’ in state 'START'.
h.connect_tcp("remote", "10809") # Connect to a remote server.
h.get_size() # Get size of the remote disk.
buf = h.pread(512, 0) # Read the first sector.
exit() or Ctrl-D # Quit the shell
help(nbd) # Display documentation
nbd> print(h)
Traceback (most recent call last):
File "/usr/lib64/python3.11/code.py", line 90, in runcode
exec(code, self.locals)
File "<console>", line 1, in <module>
NameError: name 'h' is not defined
Eww. I didn't mean for that to happen, obviously.
> I'm not sure what is the issue, but usually if you have a global
> variable created only in
> some flows, adding:
>
> thing = None
>
> At the start of the module makes sure that the name exists later,
> regardless of the flow
> taken. Code can take the right action based on:
>
> if thing is None:
> ...
Good point, I was trying 'undef h' instead. 'h' not being present in
the dictionary at all vs 'h = None' are slightly different, although I
suppose it doesn't matter in this particular case.
The other point is that 'h' (when defined) is not a global. The patch
assumes it is a global, but then uses it in some places as if it is a
local.
Do you want me to try to play with this as well (since I do like the
improved help message, if we can get it to work reliably), or would I
be duplicating efforts at this point?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
11:29 a.m.
New subject: [libnbd PATCH v2 3/3] nbdsh: Improve --help and initial banner contents.
On Tue, Jan 31, 2023 at 10:41:46AM -0600, Eric Blake wrote:
On Tue, Jan 31, 2023 at 11:52:27AM +0000, Richard W.M. Jones wrote:
> On Tue, Jan 31, 2023 at 01:33:25PM +0200, Nir Soffer wrote:
> > On Tue, Jan 31, 2023 at 12:34 AM Richard W.M. Jones <rjones(a)redhat.com>
wrote:
> > >
> > > On Fri, Nov 04, 2022 at 04:18:31PM -0500, Eric Blake wrote:
> > > > Document all options in --help output. If -n is not in use, then
> > > > enhance the banner to print the current state of h, and further
tailor
> > > > the advice given on useful next steps to take to mention opt_go when
> > > > using --opt-mode.
> > >
> > > I had to partially revert this patch (reverting most of it) because it
> > > unfortunately breaks the implicit handle creation :-(
> > >
> > >
https://gitlab.com/nbdkit/libnbd/-/commit/5a02c7d2cc6a201f9e5531c0c20c2f3...
> > >
> > > I'm not actually sure how to do this correctly in Python. I made
> > > several attempts, but I don't think Python is very good about having
a
> > > variable which is only defined on some paths -- maybe it's not
> > > possible at all.
> >
> > Can you share the error when it breaks?
>
> $ rpm -qf /usr/bin/nbdsh
> python3-libnbd-1.15.9-2.fc38.x86_64
>
> $ nbdsh
>
> Welcome to nbdsh, the shell for interacting with
> Network Block Device (NBD) servers.
>
> The ‘nbd’ module has already been imported and there
> is an open NBD handle called ‘h’ in state 'START'.
>
> h.connect_tcp("remote", "10809") # Connect to a remote
server.
> h.get_size() # Get size of the remote disk.
> buf = h.pread(512, 0) # Read the first sector.
> exit() or Ctrl-D # Quit the shell
> help(nbd) # Display documentation
>
> nbd> print(h)
> Traceback (most recent call last):
> File "/usr/lib64/python3.11/code.py", line 90, in runcode
> exec(code, self.locals)
> File "<console>", line 1, in <module>
> NameError: name 'h' is not defined
Eww. I didn't mean for that to happen, obviously.
>
> > I'm not sure what is the issue, but usually if you have a global
> > variable created only in
> > some flows, adding:
> >
> > thing = None
> >
> > At the start of the module makes sure that the name exists later,
> > regardless of the flow
> > taken. Code can take the right action based on:
> >
> > if thing is None:
> > ...
>
> Good point, I was trying 'undef h' instead. 'h' not being present
in
> the dictionary at all vs 'h = None' are slightly different, although I
> suppose it doesn't matter in this particular case.
>
> The other point is that 'h' (when defined) is not a global. The patch
> assumes it is a global, but then uses it in some places as if it is a
> local.
Do you want me to try to play with this as well (since I do like the
improved help message, if we can get it to work reliably), or would I
be duplicating efforts at this point?
I don't have any further plans here, so if you want to. However I
would say it's very subtle. In particular I don't have a clear mental
model of how variable scope works in Python.
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
475
days inactive
563
days old
15 comments
3 participants
participants (3)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones