Eric Blake
2020-Sep-22 16:15 UTC
[Libguestfs] [libnbd PATCH v2] nbdsh: Catch nbd.Error from -c arguments
When using nbdsh for scripting, it is convenient to let nbdsh fail with status 1 when encountering an API failure. However, doing so by letting the nbd.Error exception leak all the way causes ABRT (at least on Fedora 32 with abrt-python3-handler installed) to assume the program crashed from a programming error, and needlessly complicates clients to have to add try: blocks. Better is if nbdsh itself handles the problem, and only prints a stack trace when debugging is in effect, but otherwise just prints the error message. In this way, the user is not presented with a wall of python stack trace, and ABRT does not think that the exception was unhandled. See https://github.com/libguestfs/nbdkit/commit/e13048fd9 for an example of client cleanup made more verbose if we don't patch libnbd. --- On IRC, we decided that printing the stack trace can be useful when debugging (if -c triggers calls through some deeply-nested python code), but generally gets in the way for default behavior. python/nbdsh.py | 20 ++++++++++++++++---- sh/test-error.sh | 23 ++++++++++++++++++++++- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/python/nbdsh.py b/python/nbdsh.py index 61d38e8..9ed2938 100644 --- a/python/nbdsh.py +++ b/python/nbdsh.py @@ -16,6 +16,10 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +import os +import traceback + + # The NBD shell. def shell(): import argparse @@ -100,8 +104,16 @@ help(nbd) # Display documentation else: # https://stackoverflow.com/a/11754346 d = dict(locals(), **globals()) - for c in args.command: - if c != '-': - exec(c, d, d) + 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 os.environ.get("LIBNBD_DEBUG", "0") == "1": + traceback.print_exc() else: - exec(sys.stdin.read(), d, d) + print("nbdsh: command line script failed: %s" % ex.string, + file=sys.stderr) + sys.exit(1) diff --git a/sh/test-error.sh b/sh/test-error.sh index c6ab474..a33ce47 100755 --- a/sh/test-error.sh +++ b/sh/test-error.sh @@ -40,6 +40,27 @@ 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' test-error.err +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 +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 +test ! -s $out +cat $err +grep Traceback $err && fail=1 +grep '^nbdsh: command line script failed: nbd_is_read_only: ' $err exit $fail -- 2.28.0
Daniel P. Berrangé
2020-Sep-22 16:18 UTC
Re: [Libguestfs] [libnbd PATCH v2] nbdsh: Catch nbd.Error from -c arguments
On Tue, Sep 22, 2020 at 11:15:39AM -0500, Eric Blake wrote:> When using nbdsh for scripting, it is convenient to let nbdsh fail > with status 1 when encountering an API failure. However, doing so by > letting the nbd.Error exception leak all the way causes ABRT (at least > on Fedora 32 with abrt-python3-handler installed) to assume the > program crashed from a programming error, and needlessly complicates > clients to have to add try: blocks. Better is if nbdsh itself handles > the problem, and only prints a stack trace when debugging is in > effect, but otherwise just prints the error message. In this way, the > user is not presented with a wall of python stack trace, and ABRT does > not think that the exception was unhandled. > > See https://github.com/libguestfs/nbdkit/commit/e13048fd9 for an > example of client cleanup made more verbose if we don't patch libnbd. > --- > > On IRC, we decided that printing the stack trace can be useful when > debugging (if -c triggers calls through some deeply-nested python > code), but generally gets in the way for default behavior. > > python/nbdsh.py | 20 ++++++++++++++++---- > sh/test-error.sh | 23 ++++++++++++++++++++++- > 2 files changed, 38 insertions(+), 5 deletions(-)Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Richard W.M. Jones
2020-Sep-22 19:58 UTC
Re: [Libguestfs] [libnbd PATCH v2] nbdsh: Catch nbd.Error from -c arguments
On Tue, Sep 22, 2020 at 11:15:39AM -0500, Eric Blake wrote:> + 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 os.environ.get("LIBNBD_DEBUG", "0") == "1":Would it be better to use nbd.NBD().get_debug() == True, or h.get_debug() == True for this test instead of duplicating the logic inside libnbd? But sure, let's do this. ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Eric Blake
2020-Sep-22 21:03 UTC
Re: [Libguestfs] [libnbd PATCH v2] nbdsh: Catch nbd.Error from -c arguments
On 9/22/20 2:58 PM, Richard W.M. Jones wrote:> On Tue, Sep 22, 2020 at 11:15:39AM -0500, Eric Blake wrote: >> + 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 os.environ.get("LIBNBD_DEBUG", "0") == "1": > > Would it be better to use nbd.NBD().get_debug() == True, or > h.get_debug() == True for this test instead of duplicating the logic > inside libnbd?I originally worried about: nbdsh -c 'h = None' which means we can't assume that the handle we opened prior to -c code is still valid at the point we catch an exception. But you have a point that opening yet another NBD handle will work. I'll push the obvious followup.> > But sure, let's do this. ACK. > > Rich. >-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Reasonably Related Threads
- [libnbd PATCH v2 0/3] Improve nbdsh -u handling
- [libnbd PATCH] nbdsh: Hide nbd.Error from abrt-python3-handler
- Re: [libnbd PATCH v2] nbdsh: Catch nbd.Error from -c arguments
- [libnbd PATCH] nbdsh: Add -b option to simplify h.block_status
- [libnbd PATCH] nbdsh: Start adding unit tests