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

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.