Libguestfs - Jun 2019 - [PATCH libnbd v2] python: Raise a custom exception containing error string and errno.

This kind of fixes the problems in v1.  The exception still primarily
lives in the libnbdmod and you could still refer to it using
libnbdmod.Error (but don't do that).  However when the exception is
printed it now appears as nbd.Error, and you can catch it also using
the same name.

Other problems:

 - There is no "nice" interface to accessing the exception fields.
   You have to use ex.args[0] and ex.args[1].

 - The errno is represented as a number, which means it will be hard
   to write portable Python code depending on the errno.

Rich.

Previously errors caused a RuntimeException to be raised.  This commit
defines a custom exception (nbd.Error) which has two parameters, the
required error string, and the optional errno (which may be 0 if
unavailable).

For example:

$ ./run nbdsh -c 'h.pread(0, 0)'
Traceback (most recent call last):
  File "/usr/lib64/python3.7/runpy.py", line 193, in
_run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib64/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/rjones/d/libnbd/python/nbd.py", line 1136, in
<module>
    nbdsh.shell()
  File "/home/rjones/d/libnbd/python/nbdsh.py", line 62, in shell
    exec (c)
  File "<string>", line 1, in <module>
  File "/home/rjones/d/libnbd/python/nbd.py", line 456, in pread
    return libnbdmod.pread (self._o, count, offset, flags)
nbd.Error: ('nbd_pread: invalid state: START: the handle must be connected
and finished handshaking with the server: Transport endpoint is not
connected', 107)
---
 generator/generator       | 34 +++++++++++++++++++++++++++++++++-
 python/t/610-exception.py | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/generator/generator b/generator/generator
index 157a9cb..0b03054 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3337,6 +3337,19 @@ get_handle (PyObject *obj)
   return (struct nbd_handle *) PyCapsule_GetPointer(obj,
\"nbd_handle\");
 }
 
+/* nbd.Error exception. */
+extern PyObject *nbd_internal_py_Error;
+
+static inline void
+raise_exception ()
+{
+  PyObject *args = PyTuple_New (2);
+
+  PyTuple_SetItem (args, 0, PyUnicode_FromString (nbd_get_error ()));
+  PyTuple_SetItem (args, 1, PyLong_FromLong (nbd_get_errno ()));
+  PyErr_SetObject (nbd_internal_py_Error, args);
+}
+
 ";
 
   List.iter (
@@ -3390,6 +3403,9 @@ static struct PyModuleDef moduledef = {
   NULL,                  /* m_free */
 };
 
+/* nbd.Error exception. */
+PyObject *nbd_internal_py_Error;
+
 extern PyMODINIT_FUNC PyInit_libnbdmod (void);
 
 PyMODINIT_FUNC
@@ -3401,6 +3417,19 @@ PyInit_libnbdmod (void)
   if (mod == NULL)
     return NULL;
 
+  nbd_internal_py_Error = PyErr_NewExceptionWithDoc (
+    \"nbd.Error\",
+    \"Exception thrown when the underlying libnbd call fails. 
This\\n\"
+    \"exception carries a two element tuple.  The first element is
a\\n\"
+    \"printable string containing the error message.  The second\\n\"
+    \"element is the optional errno (which may be 0 in some
cases\\n\"
+    \"if the error does not correspond to a system call
failure).\\n\",
+    NULL, NULL
+  );
+  if (nbd_internal_py_Error == NULL)
+    return NULL;
+  PyModule_AddObject (mod, \"Error\", nbd_internal_py_Error);
+
   return mod;
 }
 "
@@ -3796,7 +3825,7 @@ let print_python_binding name { args; ret }     | RBool |
RErr | RFd | RInt | RInt64 -> pr "  if (ret == -1) {\n";
    | RConstString | RString -> pr "  if (ret == NULL) {\n";
   );
-  pr "    PyErr_SetString (PyExc_RuntimeError, nbd_get_error ());\n";
+  pr "    raise_exception ();\n";
   pr "    py_ret = NULL;\n";
   pr "    goto out;\n";
   pr "  }\n";
@@ -3917,6 +3946,9 @@ Read the libnbd(3) man page to find out how to use the
API.
 
 import libnbdmod
 
+# Reexport Error exception as nbd.Error
+from libnbdmod import Error
+
 ";
 
   List.iter (fun (n, i) -> pr "%-30s = %d\n" n i) constants;
diff --git a/python/t/610-exception.py b/python/t/610-exception.py
new file mode 100644
index 0000000..c7edf37
--- /dev/null
+++ b/python/t/610-exception.py
@@ -0,0 +1,33 @@
+# libnbd Python bindings
+# Copyright (C) 2010-2019 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+import nbd
+
+h = nbd.NBD ()
+
+try:
+    # This will always throw an exception because the handle is not
+    # connected.
+    h.pread (0, 0)
+except nbd.Error as ex:
+    str = ex.args[0]
+    errno = ex.args[1]
+    print ("str = %s\nerr = %d\n" % (str, errno))
+    exit (0)
+
+# If we reach here then we didn't catch the exception above.
+exit (1)
-- 
2.22.0

On 6/28/19 7:31 AM, Richard W.M. Jones wrote:
> This kind of fixes the problems in v1.  The exception still primarily
> lives in the libnbdmod and you could still refer to it using
> libnbdmod.Error (but don't do that).  However when the exception is
> printed it now appears as nbd.Error, and you can catch it also using
> the same name.
> 
> Other problems:
> 
>  - There is no "nice" interface to accessing the exception
fields.
>    You have to use ex.args[0] and ex.args[1].
I think we can make it nicer with a bit of effort - we just need to
figure out how to tie in an __init__ method that parses the args tuple
and assigns direct attributes (the args tuple will always be there,
since we inherit that from the Exception base class, but adding our own
local attributes makes access easier).  We may also want a __str__
method for sanely printing the contents of our exception.
> 
>  - The errno is represented as a number, which means it will be hard
>    to write portable Python code depending on the errno.
Not that difficult. The standard python error module can doe reverse
lookup error.errorcode[integer] to give you the string 'EINVAL' (if
integer has the same value as errno.EINVAL).

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

On 6/28/19 7:31 AM, Richard W.M. Jones wrote:
> Previously errors caused a RuntimeException to be raised.  This commit
> defines a custom exception (nbd.Error) which has two parameters, the
> required error string, and the optional errno (which may be 0 if
> unavailable).
> 
> For example:
> 
> $ ./run nbdsh -c 'h.pread(0, 0)'
> Traceback (most recent call last):
>   File "/usr/lib64/python3.7/runpy.py", line 193, in
_run_module_as_main
>     "__main__", mod_spec)
>   File "/usr/lib64/python3.7/runpy.py", line 85, in _run_code
>     exec(code, run_globals)
>   File "/home/rjones/d/libnbd/python/nbd.py", line 1136, in
<module>
>     nbdsh.shell()
>   File "/home/rjones/d/libnbd/python/nbdsh.py", line 62, in shell
>     exec (c)
>   File "<string>", line 1, in <module>
>   File "/home/rjones/d/libnbd/python/nbd.py", line 456, in pread
>     return libnbdmod.pread (self._o, count, offset, flags)
> nbd.Error: ('nbd_pread: invalid state: START: the handle must be
connected and finished handshaking with the server: Transport endpoint is not
connected', 107)
> ---
>  generator/generator       | 34 +++++++++++++++++++++++++++++++++-
>  python/t/610-exception.py | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 1 deletion(-)
> +++ b/generator/generator
> @@ -3337,6 +3337,19 @@ get_handle (PyObject *obj)
>    return (struct nbd_handle *) PyCapsule_GetPointer(obj,
\"nbd_handle\");
>  }
>  
> +/* nbd.Error exception. */
> +extern PyObject *nbd_internal_py_Error;
> +
> +static inline void
> +raise_exception ()
> +{
> +  PyObject *args = PyTuple_New (2);
> +
> +  PyTuple_SetItem (args, 0, PyUnicode_FromString (nbd_get_error ()));
> +  PyTuple_SetItem (args, 1, PyLong_FromLong (nbd_get_errno ()));
> +  PyErr_SetObject (nbd_internal_py_Error, args);
Misses error checking that PyTyple_New/PyTuple_SetItem didn't fail.

I think we want to ensure that our exception class has an __init__
method that can parse the tuple that we pass here and turn it into two
sane attributes (of course, we still want to pass that tuple to the
parent class, as all Python exceptions expect to access ex.args).
Adding helper methods to our exception class would happen...
> +}
> +
>  ";
>  
>    List.iter (
> @@ -3390,6 +3403,9 @@ static struct PyModuleDef moduledef = {
>    NULL,                  /* m_free */
>  };
>  
> +/* nbd.Error exception. */
> +PyObject *nbd_internal_py_Error;
> +
>  extern PyMODINIT_FUNC PyInit_libnbdmod (void);
>  
>  PyMODINIT_FUNC
> @@ -3401,6 +3417,19 @@ PyInit_libnbdmod (void)
>    if (mod == NULL)
>      return NULL;
>  
> +  nbd_internal_py_Error = PyErr_NewExceptionWithDoc (
> +    \"nbd.Error\",
> +    \"Exception thrown when the underlying libnbd call fails. 
This\\n\"
> +    \"exception carries a two element tuple.  The first element is
a\\n\"
> +    \"printable string containing the error message.  The
second\\n\"
> +    \"element is the optional errno (which may be 0 in some
cases\\n\"
> +    \"if the error does not correspond to a system call
failure).\\n\",
> +    NULL, NULL
> +  );
...here. I don't know if it is as simple as passing a non-NULL dict
argument that includes an entry for the appropriate '__init__' method
(for parsing our incoming tuple) and an appropriate '__str__' for sanely
printing the exception, or if it is more complicated by actually
constructing our type in C
(https://docs.python.org/3/extending/newtypes_tutorial.html#subclassing-other-types).
 And if we figure out that, we should update the docs to refer to direct
attribute names (we can bikeshed on whether to name it e.message or
e.error as the message, and e.errno as an integer that maps back to the
standard 'errno' module).

In fact, is there a way to just write the subclass in pure python, then
import that in C code, rather than trying to generate it directly in C
code? As long as we have a way to suck in the correct PyObject (well,
its subclass PyTypeObject) representing the exception subclass, that's
all the more we need for populating nbd_internal_py_Error for later
handing off to PyErr_SetObject.

> +  if (nbd_internal_py_Error == NULL)
> +    return NULL;
> +  PyModule_AddObject (mod, \"Error\", nbd_internal_py_Error);
> +
>    return mod;
>  }
>  "
> @@ -3796,7 +3825,7 @@ let print_python_binding name { args; ret } >    
| RBool | RErr | RFd | RInt | RInt64 -> pr "  if (ret == -1) {\n";
>     | RConstString | RString -> pr "  if (ret == NULL) {\n";
>    );
> -  pr "    PyErr_SetString (PyExc_RuntimeError, nbd_get_error
());\n";
> +  pr "    raise_exception ();\n";
>    pr "    py_ret = NULL;\n";
>    pr "    goto out;\n";
>    pr "  }\n";
> @@ -3917,6 +3946,9 @@ Read the libnbd(3) man page to find out how to use
the API.
>  
>  import libnbdmod
>  
> +# Reexport Error exception as nbd.Error
> +from libnbdmod import Error
> +
This part looks good.
>  ";
>  
>    List.iter (fun (n, i) -> pr "%-30s = %d\n" n i) constants;
> diff --git a/python/t/610-exception.py b/python/t/610-exception.py
> new file mode 100644
> index 0000000..c7edf37
> --- /dev/null
> +++ b/python/t/610-exception.py
> +try:
> +    # This will always throw an exception because the handle is not
> +    # connected.
> +    h.pread (0, 0)
> +except nbd.Error as ex:
> +    str = ex.args[0]
> +    errno = ex.args[1]
So, if we figure out how to tie in the right __init__ method, this would
be spelled:

  str = ex.message
  err = ex.errno
  err_name = errno.errorcode[err]
> +    print ("str = %s\nerr = %d\n" % (str, errno))
and with a decent-enough __str__, we could even just:

print (ex)

where that uses errno.errorcode to print a name rather than a number (or
omit things if the number is 0).
> +    exit (0)
> +
> +# If we reach here then we didn't catch the exception above.
> +exit (1)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org