[Libguestfs] [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks

[Richard W.M. Jones]

On Thu, Jun 09, 2022 at 08:34:43AM -0500, Eric Blake wrote:
> Instead of open-coding our code to create a PyObject wrapper of the
> mutable *error to be passed to each callback, extract this into a
> helper function.  We can then slightly optimize things to hang on to a
> single pointer to the ctypes module, rather than looking it up on
> every callback.  The generated code shrinks by more than the added
> code in utils.c:
> 
> | --- python/methods.h.bak	2022-06-08 14:35:00.454786844 -0500
> | +++ python/methods.h	2022-06-08 14:43:24.681596853 -0500
> | @@ -35,6 +35,7 @@
> |  extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
> |  extern int nbd_internal_py_init_aio_buffer (PyObject *);
> |  extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
> | +extern PyObject *nbd_internal_py_wrap_errptr (int);
> |
> |  static inline struct nbd_handle *
> |  get_handle (PyObject *obj)
> | --- python/methods.c.bak	2022-06-08 14:35:00.450786838 -0500
> | +++ python/methods.c	2022-06-08 14:43:24.696596877 -0500
> | @@ -75,13 +75,7 @@ chunk_wrapper (void *user_data, const vo
> |    PyObject *py_args, *py_ret;
> |    PyObject *py_error = NULL;
> |
> | -  PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
> | -  if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
> | -  PyObject *py_error_mod = PyImport_Import (py_error_modname);
> | -  Py_DECREF (py_error_modname);
> | -  if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
> | -  py_error = PyObject_CallMethod (py_error_mod, "c_int", "i", *error);
> | -  Py_DECREF (py_error_mod);
> | +  py_error = nbd_internal_py_wrap_errptr (*error);
> |    if (!py_error) { PyErr_PrintEx (0); goto out; }
> |
> |    py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status,
> | @@ -132,13 +126,7 @@ completion_wrapper (void *user_data, int
> |    PyObject *py_args, *py_ret;
> |    PyObject *py_error = NULL;
> |
> | -  PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
> | -  if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
> | -  PyObject *py_error_mod = PyImport_Import (py_error_modname);
> | -  Py_DECREF (py_error_modname);
> | -  if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
> | -  py_error = PyObject_CallMethod (py_error_mod, "c_int", "i", *error);
> | -  Py_DECREF (py_error_mod);
> | +  py_error = nbd_internal_py_wrap_errptr (*error);
> |    if (!py_error) { PyErr_PrintEx (0); goto out; }
> |
> |    py_args = Py_BuildValue ("(O)", py_error);
> | @@ -239,13 +227,7 @@ extent_wrapper (void *user_data, const c
> |      if (!py_e_entries) { PyErr_PrintEx (0); goto out; }
> |      PyList_SET_ITEM (py_entries, i_entries, py_e_entries);
> |    }
> | -  PyObject *py_error_modname = PyUnicode_FromString ("ctypes");
> | -  if (!py_error_modname) { PyErr_PrintEx (0); goto out; }
> | -  PyObject *py_error_mod = PyImport_Import (py_error_modname);
> | -  Py_DECREF (py_error_modname);
> | -  if (!py_error_mod) { PyErr_PrintEx (0); goto out; }
> | -  py_error = PyObject_CallMethod (py_error_mod, "c_int", "i", *error);
> | -  Py_DECREF (py_error_mod);
> | +  py_error = nbd_internal_py_wrap_errptr (*error);
> |    if (!py_error) { PyErr_PrintEx (0); goto out; }
> |
> |    py_args = Py_BuildValue ("(sKOO)", metacontext, offset, py_entries,
> ---
>  generator/Python.ml |  9 ++-------
>  python/utils.c      | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/generator/Python.ml b/generator/Python.ml
> index e94f37b..6980034 100644
> --- a/generator/Python.ml
> +++ b/generator/Python.ml
> @@ -41,6 +41,7 @@ let
>  extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool);
>  extern int nbd_internal_py_init_aio_buffer (PyObject *);
>  extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
> +extern PyObject *nbd_internal_py_wrap_errptr (int);
> 
>  static inline struct nbd_handle *
>  get_handle (PyObject *obj)
> @@ -184,13 +185,7 @@ let
>      | CBInt _
>      | CBInt64 _ -> ()
>      | CBMutable (Int n) ->
> -       pr "  PyObject *py_%s_modname = PyUnicode_FromString (\"ctypes\");\n" n;
> -       pr "  if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n" n;
> -       pr "  PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n;
> -       pr "  Py_DECREF (py_%s_modname);\n" n;
> -       pr "  if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n;
> -       pr "  py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n;
> -       pr "  Py_DECREF (py_%s_mod);\n" n;
> +       pr "  py_%s = nbd_internal_py_wrap_errptr (*%s);\n" n n;
>         pr "  if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n;
>      | CBString _
>      | CBUInt _
> diff --git a/python/utils.c b/python/utils.c
> index e0df181..cd44b90 100644
> --- a/python/utils.c
> +++ b/python/utils.c
> @@ -173,3 +173,22 @@ nbd_internal_py_get_nbd_buffer_type (void)
>    }
>    return type;
>  }
> +
> +/* Helper to package callback *error into modifiable PyObject */
> +PyObject *
> +nbd_internal_py_wrap_errptr (int err)
> +{
> +  static PyObject *py_ctypes_mod;
> +
> +  if (!py_ctypes_mod) {
> +    PyObject *py_modname = PyUnicode_FromString ("ctypes");
> +    if (!py_modname)
> +      return NULL;
> +    py_ctypes_mod = PyImport_Import (py_modname);
> +    Py_DECREF (py_modname);
> +    if (!py_ctypes_mod)
> +      return NULL;
> +  }
> +
> +  return PyObject_CallMethod (py_ctypes_mod, "c_int", "i", err);
> +}

I guess because of the Python GIL we don't need locking here?

We don't run the Python tests with valgrinding so there should be no
issue there.

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org