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