[libvirt] libvirt-python bug: custom event loop impl calls ff callback from *Remove(Handle|Timeout)Func

[Daniel P. Berrange]

On Thu, Jan 19, 2017 at 12:48:11PM +0100, Wojtek Porczyk wrote:
> Hello libvirt-list,
> 
> As of current libvirt-python.git, according to libvirt-override.c, if
> implementing custom event loop in Python, ff callback is called from
> libvirt_virEventRemoveHandleFunc, which is a C glue between
> virEventRegisterImpl and actual removeHandle function written in Python:
> 
> > result = PyEval_CallObject(removeHandleObj, pyobj_args);
> > if (!result) {
> >     PyErr_Print();
> >     PyErr_Clear();
> > } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
> >     DEBUG("%s: %s must return opaque obj registered with %s"
> >           "to avoid leaking libvirt memory\n",
> >           __FUNCTION__, NAME(removeHandle), NAME(addHandle));
> > } else {
> >     opaque = PyTuple_GetItem(result, 1);
> >     ff = PyTuple_GetItem(result, 2);
> >     cff = PyvirFreeCallback_Get(ff);
> >     if (cff)
> >         (*cff)(PyvirVoidPtr_Get(opaque));
> >     retval = 0;
> > }
> 
> This is exactly what one shoud not be doing according to documentation [1]:
> 
> > If the opaque user data requires free'ing when the handle is unregistered,
> > then a 2nd callback can be supplied for this purpose. This callback needs to
> > be invoked from a clean stack. If 'ff' callbacks are invoked directly from the
> > virEventRemoveHandleFunc they will likely deadlock in libvirt.
> 
> [1] https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc
> 
> This is true, the deadlock occurs. When the "result" tuple is mangled to have
> None as third item ("ff"), then cff = PyvirFreeCallback_Get(ff) is NULL and
> the deadlock does not happen.
> 
> That's also why script examples/event-test.py does not deadlock, because it
> does not return anything (that is, returns None) from Python, so the second if
> block happens and the ff callback, if any, is not executed (and probably
> something leaks, but I didn't check for that).
> 
> 
> Everything also applies to to timeouts (libvirt_virEventRemoteTimeoutFunc).

Yes, your analysis looks correct to me. I don't think it is easy to fix
this in the libvirt-override.c file really, unless we were to spawn a throw
away thread to call "ff" in. The better approach would be for the user suppied
event loop impl of removeHandle to arrange to call 'ff' themselves at the
correct time.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|