thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH v3 1/5] python: Simplify passing of mutable *error to callbacks [Jun 2022]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2022-Jun-09 14:22 UTC

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

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

Eric Blake

2022-Jun-09 16:03 UTC

head link

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

On Thu, Jun 09, 2022 at 03:22:42PM +0100, Richard W.M. Jones
wrote:> 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:
> > 
> > +++ 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;
The old code was using PyObject_CallMethod outside the GIL lock...
> > +       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;
...the new code as well.
> > +  }
> > +
> > +  return PyObject_CallMethod (py_ctypes_mod, "c_int",
"i", err);
> > +}
> 
> I guess because of the Python GIL we don't need locking here?
But you're right that any time we call into the python interpreter for
something that may entail quite some level of complexity, the GIL lock
needs to be worried about.  I'll have to audit this and other recent
patches to see if we need to add more use of
PyGILState_{Ensure/Release} around various code blocks (after first
researching what the lock is supposed to do...).
> 
> We don't run the Python tests with valgrinding so there should be no
> issue there.
It's a reference we never release (same thing done in recent commit
9c5b0eab) - if valgrind can see that static storage is still
reachable, rather than classing it as a leak, then even if we do run
under valgrind we'd be okay.  If not, it's provable this is not a
long-term leak, so we could add in a suppression rule.

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

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

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

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