Libguestfs - Sep 2020 - [libnbd PATCH] nbdsh: Hide nbd.Error from abrt-python3-handler

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 (on
Fedora 32 at least) to assume the program crashed from a programming
error, and needlessly complicates clients to have to add try: blocks.
Better is if nbdsh itself prints the same stack trace that python
would, but handles the error and exits python cleanly so that 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.
---

Is it worth a command-line option, off by default, but that the user
can opt in to, where the option being set says we re-raise the excpetion
instead of handling it ourselves?

 python/nbdsh.py  | 18 ++++++++++++------
 sh/Makefile.am   |  4 +++-
 sh/test-error.sh | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 7 deletions(-)
 create mode 100755 sh/test-error.sh

diff --git a/python/nbdsh.py b/python/nbdsh.py
index c9a7beb..93d4998 100644
--- a/python/nbdsh.py
+++ b/python/nbdsh.py
@@ -1,5 +1,5 @@
 # NBD client library in userspace
-# Copyright (C) 2013-2019 Red Hat Inc.
+# Copyright (C) 2013-2020 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
@@ -16,6 +16,9 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA


+import traceback
+
+
 # The NBD shell.
 def shell():
     import argparse
@@ -95,8 +97,12 @@ 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)
-            else:
-                exec(sys.stdin.read(), d, d)
+        try:
+            for c in args.command:
+                if c != '-':
+                    exec(c, d, d)
+                else:
+                    exec(sys.stdin.read(), d, d)
+        except nbd.Error:
+            traceback.print_exc()
+            sys.exit(1)
diff --git a/sh/Makefile.am b/sh/Makefile.am
index 415e241..8c4b982 100644
--- a/sh/Makefile.am
+++ b/sh/Makefile.am
@@ -1,5 +1,5 @@
 # nbd client library in userspace
-# Copyright (C) 2013-2019 Red Hat Inc.
+# Copyright (C) 2013-2020 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
@@ -22,6 +22,7 @@ EXTRA_DIST = \
 	examples/LICENSE-FOR-EXAMPLES \
 	examples/hexdump.sh \
 	test-context.sh \
+	test-error.sh \
 	test-help.sh \
 	test-pattern.sh \
 	test-version.sh \
@@ -49,6 +50,7 @@ LOG_COMPILER = $(top_builddir)/run
 TESTS = \
 	test-help.sh \
 	test-version.sh \
+	test-error.sh \
 	$(NULL)

 if HAVE_NBDKIT
diff --git a/sh/test-error.sh b/sh/test-error.sh
new file mode 100755
index 0000000..d8a1d84
--- /dev/null
+++ b/sh/test-error.sh
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2019-2020 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
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+# Test how nbdsh -c handles errors
+fail=0
+
+. ../tests/functions.sh
+
+files='test-error.out test-error.err'
+cleanup_fn rm -f $files
+rm -f $files
+
+# Triggering nbd.Error non-interactively prints the error
+nbdsh -c 'h.is_read_only()' > test-error.out 2> test-error.err
&& fail=1
+test ! -s test-error.out
+cat test-error.err
+grep Traceback test-error.err
+grep 'in is_read_only' test-error.err
+grep '^nbd\.Error: nbd_is_read_only: ' test-error.err
+
+exit $fail
-- 
2.28.0

On 9/18/20 9:42 AM, 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 (on
> Fedora 32 at least) to assume the program crashed from a programming
> error, and needlessly complicates clients to have to add try: blocks.
> Better is if nbdsh itself prints the same stack trace that python
> would, but handles the error and exits python cleanly so that 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.
I missed one: 'nbdsh -u garbage_uri' lets nbd.Error escape from a failed
nbd_connect_uri.  We may want to treat that one differently than we do 
the concatenated -c arguments, but it's another one worth treating, once 
we figure out if this is the right thing to do from the ABRT perspective.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org