Libguestfs - Apr 2018 - Re: [PATCH nbdkit] python: Turn python exceptions into nbdkit errors properly.

Much more annoying that it needs to be, but I have tested it and it
works on Python 2 & 3.  Note this will not work on Python 3.0 - 3.2,
but I guess we don't care about those versions.

Rich.

---
 configure.ac                   |  8 +++++++
 plugins/python/python.c        | 51 +++++++++++++++++++++++++++++++++++++++---
 tests/Makefile.am              |  1 +
 tests/python-exception.py      | 45 +++++++++++++++++++++++++++++++++++++
 tests/test-python-exception.sh | 42 ++++++++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index be38c84..c4c04d3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -270,6 +270,14 @@ AS_IF([test "x$PYTHON" != "xno"
&& test "x$enable_python" != "xno"],[
                  [AC_DEFINE([HAVE_PYSTRING_FROMSTRING],1,
                             [Found PyString_FromString in libpython.])],
                  [],[$PYTHON_BLDLIBRARY])
+    AC_CHECK_LIB([c],[PyString_AsString],
+                 [AC_DEFINE([HAVE_PYSTRING_ASSTRING],1,
+                            [Found PyString_AsString in libpython.])],
+                 [],[$PYTHON_BLDLIBRARY])
+    AC_CHECK_LIB([c],[PyUnicode_AsUTF8],
+                 [AC_DEFINE([HAVE_PYUNICODE_ASUTF8],1,
+                            [Found PyUnicode_AsUTF8 in libpython.])],
+                 [],[$PYTHON_BLDLIBRARY])
 
     LIBS="$old_LIBS"
 
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 83a32ea..02f0d4c 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -103,13 +103,58 @@ callback_defined (const char *name, PyObject **obj_rtn)
   return 1;
 }
 
+/* Convert bytes/str/unicode into a string.  Caller must free. */
+static char *
+python_to_string (PyObject *str)
+{
+  char *r;
+
+  if (str) {
+#ifdef HAVE_PYUNICODE_ASUTF8
+    if (PyUnicode_Check (str)) {
+      r = PyUnicode_AsUTF8 (str);
+      r = strdup (r);
+      return r;
+    }
+    else
+#endif
+#ifdef HAVE_PYSTRING_ASSTRING
+    if (PyString_Check (str)) {
+      r = PyString_AsString (str); /* Internal pointer in PyObject. */
+      r = strdup (r);
+      return r;
+    }
+    else
+#endif
+    if (PyBytes_Check (str)) {
+      r = PyBytes_AS_STRING (str);
+      r = strdup (r);
+      return r;
+    }
+  }
+  return NULL;
+}
+
 static int
 check_python_failure (const char *callback)
 {
   if (PyErr_Occurred ()) {
-    nbdkit_error ("%s: callback failed: %s", script, callback);
-    /* How to turn this into a string? XXX */
-    PyErr_Print ();
+    PyObject *type, *error, *traceback, *error_str;
+    char *error_cstr;
+
+    /* Convert the Python exception to a string.
+     * https://stackoverflow.com/a/1418703
+     * But forget about the traceback, it's very hard to print.
+     * https://stackoverflow.com/q/1796510
+     */
+    PyErr_Fetch (&type, &error, &traceback);
+    PyErr_NormalizeException (&type, &error, &traceback);
+    error_str = PyObject_Str (error);
+    error_cstr = python_to_string (error_str);
+    nbdkit_error ("%s: %s: error: %s",
+                  script, callback, error_cstr ? :
"<unknown>");
+    Py_DECREF (error_str);
+    free (error_cstr);
     return -1;
   }
   return 0;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 56a20f6..6cb6344 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -383,6 +383,7 @@ if HAVE_PYTHON
 check_PROGRAMS += test-python
 TESTS += \
 	test-python \
+	test-python-exception.sh \
 	test-shebang-python.sh
 
 test_python_SOURCES = test-lang-plugins.c test.h
diff --git a/tests/python-exception.py b/tests/python-exception.py
new file mode 100644
index 0000000..93531cc
--- /dev/null
+++ b/tests/python-exception.py
@@ -0,0 +1,45 @@
+# nbdkit
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# A dummy python plugin which just raises an exception in config_complete.
+
+def config_complete():
+    raise RuntimeError("this is the test string")
+
+def open(readonly):
+    return 1
+
+def get_size(h):
+    return 0
+
+def pread(h, count, offset):
+    return ""
diff --git a/tests/test-python-exception.sh b/tests/test-python-exception.sh
new file mode 100755
index 0000000..83999af
--- /dev/null
+++ b/tests/test-python-exception.sh
@@ -0,0 +1,42 @@
+#!/bin/bash -
+# nbdkit
+# Copyright (C) 2018 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+output=test-python-exception.out
+
+rm -f $output
+
+nbdkit -f -v python ./python-exception.py > $output 2>&1 ||:
+
+grep 'this is the test string' $output
+
+rm $output
-- 
2.16.2

On Thu, Apr 05, 2018 at 01:47:30PM +0100, Richard W.M. Jones
wrote:
> Much more annoying that it needs to be, but I have tested it and it
> works on Python 2 & 3.  Note this will not work on Python 3.0 - 3.2,
> but I guess we don't care about those versions.
What's the problem that impacts 3.0 -> 3.2 ?   I don't particularly
care, just curious really.  Perhaps mention the problem in the commit
message as a hint for anyone who does care

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 04/05/2018 07:47 AM, Richard W.M. Jones wrote:
> ---
>  configure.ac                   |  8 +++++++
>  plugins/python/python.c        | 51
+++++++++++++++++++++++++++++++++++++++---
>  tests/Makefile.am              |  1 +
>  tests/python-exception.py      | 45 +++++++++++++++++++++++++++++++++++++
>  tests/test-python-exception.sh | 42 ++++++++++++++++++++++++++++++++++
>  5 files changed, 144 insertions(+), 3 deletions(-)
> 
> +/* Convert bytes/str/unicode into a string.  Caller must free. */
> +static char *
> +python_to_string (PyObject *str)
> +{
> +  char *r;
> +
> +  if (str) {
> +#ifdef HAVE_PYUNICODE_ASUTF8
> +    if (PyUnicode_Check (str)) {
> +      r = PyUnicode_AsUTF8 (str);
> +      r = strdup (r);
> +      return r;
Any simpler to just write:
return strdup (PyUnicode_AsUTF8 (str));

instead of using the temporary variable r?

>  static int
>  check_python_failure (const char *callback)
>  {
>    if (PyErr_Occurred ()) {
> -    nbdkit_error ("%s: callback failed: %s", script, callback);
> -    /* How to turn this into a string? XXX */
> -    PyErr_Print ();
> +    PyObject *type, *error, *traceback, *error_str;
> +    char *error_cstr;
> +
> +    /* Convert the Python exception to a string.
> +     * https://stackoverflow.com/a/1418703
> +     * But forget about the traceback, it's very hard to print.
> +     * https://stackoverflow.com/q/1796510
> +     */
> +    PyErr_Fetch (&type, &error, &traceback);
> +    PyErr_NormalizeException (&type, &error, &traceback);
> +    error_str = PyObject_Str (error);
> +    error_cstr = python_to_string (error_str);
> +    nbdkit_error ("%s: %s: error: %s",
> +                  script, callback, error_cstr ? :
"<unknown>");
Okay, not the first use of that gcc extension.

Looks reasonable to me.

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

On Thu, Apr 05, 2018 at 02:05:12PM +0100, Daniel P. Berrangé
wrote:
> On Thu, Apr 05, 2018 at 01:47:30PM +0100, Richard W.M. Jones wrote:
> > Much more annoying that it needs to be, but I have tested it and it
> > works on Python 2 & 3.  Note this will not work on Python 3.0 -
3.2,
> > but I guess we don't care about those versions.
> 
> What's the problem that impacts 3.0 -> 3.2 ?   I don't
particularly
> care, just curious really.  Perhaps mention the problem in the commit
> message as a hint for anyone who does care
It's just that PyUnicode_AsUTF8 was added in 3.3.
(https://docs.python.org/3/c-api/unicode.html)

We could add a workaround fairly easily, but I don't think we
care about this range of old versions now.

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