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

[Martin Kletzander]

On Thu, Jan 19, 2017 at 01:18:12PM +0100, Wojtek Porczyk wrote:
>On Thu, Jan 19, 2017 at 11:56:36AM +0000, Daniel P. Berrange wrote:
>> On Thu, Jan 19, 2017 at 12:48:11PM +0100, Wojtek Porczyk wrote:
>> > Hello libvirt-list,
>(snip)
>> > > } 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
>> >
>(snip)
>>
>> 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.
>
>IIUC, this cannot be done without involving user-supplied event loop at all.
>Unless I miss something (I'm not a C programmer really), that's the
>implication of "This callback needs to be invoked from a clean stack", since
>libvirt-override.c has access to "clean stack" only via whatever is
>implemented in python.
>
>That unfortunately means the python API (specifically the expected addHandle
>signature) should be amended to include explicit ff callback to be called
>directly from the loop after removeHandleFunc returns. That's not as bad as
>it sounds, because the argument can be optional in Python, so new Python code
>could be compatible with both old and new libvirt-python, and
>libvirt-override.c could catch TypeError exception to retry the function with
>different signature for existing code. That would still work not worse than
>present, since it will still leak memory. In any case, the ff callback should
>be dropped from libvirt_virEventRemoveHandleFunc(), because again, I suppose
>all currently running implementations leak like a sieve.
>
>I have no idea how to write that in C.
>

Would you mind creating a bug in our bugzilla [1] just so we don't lose
track of this issue?  Thank you very much in advance.

Martin

[1] https://bugzilla.redhat.com

>
>--
>pozdrawiam / best regards       _.-._
>Wojtek Porczyk               .-^'   '^-.
>Invisible Things Lab         |'-.-^-.-'|
>                             |  |   |  |
> I do not fear computers,    |  '-.-'  |
> I fear lack of them.        '-._ :  ,-'
>    -- Isaac Asimov             `^-^-_>



>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170203/bbc77423/attachment-0001.sig>