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

[Richard W.M. Jones]

On Thu, Jun 09, 2022 at 03:58:45PM -0500, Eric Blake wrote:
> 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.

FWIW the libguestfs python bindings allow threads and have callbacks
(back to C) which release the GIL, so may be a good example to follow.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top