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

[Eric Blake]

On Thu, Jun 09, 2022 at 11:03:02AM -0500, Eric Blake wrote:
> 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...).

Okay, after reading more about the GIL, it looks like we are (likely)
safe only because we have never really been multi-threaded: none of
our libnbd code uses Py_BEGIN_ALLOW_THREADS to release the GIL around
our calls into libnbd C code, and therefore even though our placement
of PyGILState_Ensure() was too late, all of our callbacks have been
reached from places where we already had the GIL (well, hopefully
that's true).  At any rate, I'm working on a patch to explicitly
release the GIL around calls into libnbd C code, which means we DO
need to fix our callback wrappers to grab it at the right point.  But
the code in utils.c is indeed safe because of the GIL.

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