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

[Daniel Veillard]

On Mon, Jun 20, 2011 at 03:26:18PM -0400, Cole Robinson wrote:
> 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.

  Okay, ACK :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/