Libguestfs - Sep 2020 - Re: [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

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 :|

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/

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