[libvirt] [PATCH 09/10] python: events: Fix C->Python handle callback prototype

[Cole Robinson]

On 06/19/2011 09:44 PM, Daniel Veillard wrote:
> On Wed, Jun 15, 2011 at 09:23:18PM -0400, Cole Robinson wrote:
>> If registering our own event loop implementation written in python,
>> any handles or timeouts callbacks registered by libvirt C code must
>> are wrapped in a python function. There is some argument trickery that
> 
>   "must be" :-)
> 
>> makes this all work, by wrapping the user passed opaque value in
>> a tuple, along with the callback function.
>>
>> Problem is, the current setup requires the user's event loop to know
>> about this trickery, rather than just treating the opaque value
>> as truly opaque.
>>
>> Fix this in a backwards compatible manner, and adjust the example
>> python event loop to do things the proper way.
>>
>> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>> ---
>>  examples/domain-events/events-python/event-test.py |    6 +---
>>  python/libvirt-override.py                         |   26 ++++++++++++++++++-
>>  2 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py
>> index df75dce..76fda2b 100644
>> --- a/examples/domain-events/events-python/event-test.py
>> +++ b/examples/domain-events/events-python/event-test.py
>> @@ -66,8 +66,7 @@ class virEventLoopPure:
>>              self.cb(self.handle,
>>                      self.fd,
>>                      events,
>> -                    self.opaque[0],
>> -                    self.opaque[1])
>> +                    self.opaque)
>>  
>>      # This class contains the data we need to track for a
>>      # single periodic timer
>> @@ -96,8 +95,7 @@ class virEventLoopPure:
>>  
>>          def dispatch(self):
>>              self.cb(self.timer,
>> -                    self.opaque[0],
>> -                    self.opaque[1])
>> +                    self.opaque)
>>  
>>  
>>      def __init__(self):
>> diff --git a/python/libvirt-override.py b/python/libvirt-override.py
>> index b611ca4..8df9dc1 100644
>> --- a/python/libvirt-override.py
>> +++ b/python/libvirt-override.py
>> @@ -117,19 +117,41 @@ def getVersion (name = None):
>>  #
>>  # Invoke an EventHandle callback
>>  #
>> -def eventInvokeHandleCallback (watch, fd, event, callback, opaque):
>> +def eventInvokeHandleCallback(watch, fd, event, opaque, opaquecompat=None):
>>      """
>>      Invoke the Event Impl Handle Callback in C
>>      """
>> +    # libvirt 0.9.2 and earlier required custom event loops to know
>> +    # that opaque=(cb, original_opaque) and pass the values individually
>> +    # to this wrapper. This should handle the back compat case, and make
>> +    # future invocations match the virEventHandleCallback prototype
>> +    if opaquecompat:
>> +        callback = opaque
>> +        opaque = opaquecompat
>> +    else:
>> +        callback = opaque[0]
>> +        opaque = opaque[1]
>> +
>>      libvirtmod.virEventInvokeHandleCallback(watch, fd, event, callback, opaque);
> 
>   I would rather use dynamic type test like
>     if type(callback) == type(()) and type(callback[0]) == type(lambda x:x):
> 
> than rely on an extra argument detection
> Note: I didn't tried, I don't know if python type will allow the test
> for functions and lambda to work, otherwise use a predefined function
> or another one from the module.

Part of the problem is we need to preserve the old number of function
arguments, so that an old event loop works with newer libvirt, otherwise user
will pass 5 args where only 4 allowed.

Also we can't only do a dynamic test of the opaque value, since the original
opaque value could easily be opaque=(somefunc, somedata) and we would end up
incorrectly matching that. So checking opaquecompat has to factor into it in
some way, and if we are doing that I don't think there is much benefit to
poking into the values, since if they don't match our expectations there isn't
much we can do to work around it, so better to just let it error out naturally
IMO.

Thanks,
Cole