[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] fix python events



Doh! ... attached :-)


On Fri, 2008-11-21 at 10:30 +0100, Jim Meyering wrote:
> David Lively <dlively virtualiron com> wrote:
> > This patch gets python events working again after upstream changes, and
> > make the test implementation properly clean up after itself and
> > implement the new EventImpl API properly.
> >
> > Note that the Python RemoveHandle and RemoveTimeout implementations
> > should return the opaque object registered by the corresponding
> > AddHandle/Timeout calls, in lieu of calling the (C) freefunc.  (The
> > binding code will then call freefunc if it's not NULL.)  Ignoring this
> > means you'll leak memory in the same way that C RemoveHandle/Timeout
> > leak if they don't (now) call the freefunc.
> >
> > I also moved around some of the error checking code to unclutter (and
> > speed up) the common code paths.  For instance, we now check that the
> > virRegisterEventImpl arguments are callable just once (and return
> > failure if they're not), rather than checking them before every call and
> > blithely ignoring them if they're not callable.
> >
> > Dave
> >
> >  examples/domain-events/events-python/event-test.py |   29 +--
> >  python/libvir.c                                    |  200+++++++++++++++----
> >  python/libvir.py                                   |    4
> >  python/libvirt_wrap.h                              |    8
> >  python/types.c                                     |    1
> >  python/virConnect.py                               |    4
> >  6 files changed, 194 insertions(+), 52 deletions(-)
> 
> Hi Dave,
> 
> It looks like this patch didn't make it to the list.
commit efd5098e9a834562cddbf1618e36eb43c272f8ea
Author: David Lively <dlively virtualiron com>
Date:   Thu Nov 20 16:36:14 2008 -0500

    vi-patch: fix-python-events
    
    Get python events working again after upstream changes, and make the
    test implementation properly clean up after itself and implement the
    new EventImpl properly.

diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py
index 45aaa93..1ad436f 100644
--- a/examples/domain-events/events-python/event-test.py
+++ b/examples/domain-events/events-python/event-test.py
@@ -72,22 +72,22 @@ def myAddHandle(fd, events, cb, opaque):
     h_events = events
     h_cb = cb
     h_opaque = opaque
-
     mypoll.register(fd, myEventHandleTypeToPollEvent(events))
+    return 0
 
 def myUpdateHandle(watch, event):
     global h_fd, h_events
-    #print "Updating Handle %s %s" % (str(fd), str(events))
-    h_fd = fd
+    #print "Updating Handle %s %s" % (str(h_fd), str(event))
     h_events = event
-    mypoll.unregister(watch)
-    mypoll.register(watch, myEventHandleTypeToPollEvent(event))
+    mypoll.unregister(h_fd)
+    mypoll.register(h_fd, myEventHandleTypeToPollEvent(event))
 
 def myRemoveHandle(watch):
     global h_fd
-    #print "Removing Handle %s" % str(fd)
+    #print "Removing Handle %s" % str(h_fd)
+    mypoll.unregister(h_fd)
     h_fd = 0
-    mypoll.unregister(watch)
+    return h_opaque
 
 def myAddTimeout(timeout, cb, opaque):
     global t_active, t_timeout, t_cb, t_opaque
@@ -96,16 +96,18 @@ def myAddTimeout(timeout, cb, opaque):
     t_timeout = timeout;
     t_cb = cb;
     t_opaque = opaque;
+    return 0
 
 def myUpdateTimeout(timer, timeout):
     global t_timeout
-    #print "Updating Timeout %s" % (str(timer), str(timeout))
+    #print "Updating Timeout %s %s" % (str(timer), str(timeout))
     t_timeout = timeout;
 
 def myRemoveTimeout(timer):
     global t_active
     #print "Removing Timeout %s" % str(timer)
     t_active = 0;
+    return t_opaque
 
 ##########################################
 # Main
@@ -143,6 +145,14 @@ def main():
                                myRemoveTimeout );
     vc = libvirt.open(uri)
 
+    # Close connection on exit (to test cleanup paths)
+    old_exitfunc = getattr(sys, 'exitfunc', None)
+    def exit():
+        print "Closing " + str(vc)
+        vc.close()
+        if (old_exitfunc): old_exitfunc()
+    sys.exitfunc = exit
+
     #Add 2 callbacks to prove this works with more than just one
     vc.domainEventRegister(myDomainEventCallback1,None)
     vc.domainEventRegister(myDomainEventCallback2,None)
@@ -175,8 +185,7 @@ def main():
 
         if h_cb != None:
             #print "Invoking Handle CB"
-            h_cb(0, h_fd, myPollEventToEventHandleType(revents & h_events),
-                 h_opaque[0], h_opaque[1])
+            h_cb(0, h_fd, myPollEventToEventHandleType(revents & h_events), h_opaque[0], h_opaque[1])
 
         #print "DEBUG EXIT"
         #break
diff --git a/python/libvir.c b/python/libvir.c
index 07ed09e..6ae7cc1 100644
--- a/python/libvir.c
+++ b/python/libvir.c
@@ -35,6 +35,18 @@ extern void initcygvirtmod(void);
 #define VIR_PY_INT_FAIL (libvirt_intWrap(-1))
 #define VIR_PY_INT_SUCCESS (libvirt_intWrap(0))
 
+static char *py_str(PyObject *obj)
+{
+    PyObject *str = PyObject_Str(obj);
+    if (!str) {
+        PyErr_Print();
+        PyErr_Clear();
+        return NULL;
+    };
+    return PyString_AsString(str);
+}
+        
+
 /************************************************************************
  *									*
  *		Statistics						*
@@ -484,7 +496,8 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx, virErrorPtr err)
     PyObject *result;
 
 #ifdef DEBUG_ERROR
-    printf("libvirt_virErrorFuncHandler(%p, %s, ...) called\n", ctx, msg);
+    printf("libvirt_virErrorFuncHandler(%p, %s, ...) called\n", ctx,
+           err->message);
 #endif
 
     if ((err == NULL) || (err->code == VIR_ERR_OK))
@@ -1696,11 +1709,17 @@ libvirt_virConnectDomainEventDeregister(ATTRIBUTE_UNUSED PyObject * self,
  * Event Impl
  *******************************************/
 static PyObject *addHandleObj     = NULL;
+static char *addHandleName        = NULL;
 static PyObject *updateHandleObj  = NULL;
+static char *updateHandleName     = NULL;
 static PyObject *removeHandleObj  = NULL;
+static char *removeHandleName     = NULL;
 static PyObject *addTimeoutObj    = NULL;
+static char *addTimeoutName       = NULL;
 static PyObject *updateTimeoutObj = NULL;
+static char *updateTimeoutName    = NULL;
 static PyObject *removeTimeoutObj = NULL;
+static char *removeTimeoutName    = NULL;
 
 
 static int
@@ -1710,13 +1729,14 @@ libvirt_virEventAddHandleFunc  (int fd,
                                 void *opaque,
                                 virFreeCallback ff)
 {
-    PyObject *result = NULL;
+    PyObject *result;
     PyObject *python_cb;
     PyObject *cb_obj;
     PyObject *ff_obj;
     PyObject *opaque_obj;
     PyObject *cb_args;
     PyObject *pyobj_args;
+    int retval = -1;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
@@ -1725,9 +1745,18 @@ libvirt_virEventAddHandleFunc  (int fd,
                                      "eventInvokeHandleCallback");
     if(!python_cb) {
 #if DEBUG_ERROR
-        printf("%s Error finding eventInvokeHandleCallback\n", __FUNCTION__);
+        printf("%s: Error finding eventInvokeHandleCallback\n", __FUNCTION__);
 #endif
         PyErr_Print();
+        PyErr_Clear();
+        goto cleanup;
+    }
+    if (!PyCallable_Check(python_cb)) {
+#if DEBUG_ERROR
+        char *name = py_str(python_cb);
+        printf("%s: %s is not callable\n", __FUNCTION__, name);
+        free(name);
+#endif
         goto cleanup;
     }
     Py_INCREF(python_cb);
@@ -1748,8 +1777,17 @@ libvirt_virEventAddHandleFunc  (int fd,
     PyTuple_SetItem(pyobj_args, 2, python_cb);
     PyTuple_SetItem(pyobj_args, 3, cb_args);
 
-    if(addHandleObj && PyCallable_Check(addHandleObj))
-        result = PyEval_CallObject(addHandleObj, pyobj_args);
+    result = PyEval_CallObject(addHandleObj, pyobj_args);
+    if (!result) {
+        PyErr_Print();
+        PyErr_Clear();
+    } else if (!PyInt_Check(result)) {
+#if DEBUG_ERROR
+        printf("%s: %s should return an int\n", addHandleName, __FUNCTION__);
+#endif
+    } else {
+        retval = (int)PyInt_AsLong(result);
+    }
 
     Py_XDECREF(result);
     Py_DECREF(pyobj_args);
@@ -1757,23 +1795,26 @@ libvirt_virEventAddHandleFunc  (int fd,
 cleanup:
     LIBVIRT_RELEASE_THREAD_STATE;
 
-    return 0;
+    return retval;
 }
 
 static void
-libvirt_virEventUpdateHandleFunc(int fd, int event)
+libvirt_virEventUpdateHandleFunc(int watch, int event)
 {
-    PyObject *result = NULL;
+    PyObject *result;
     PyObject *pyobj_args;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
     pyobj_args = PyTuple_New(2);
-    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd));
+    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(watch));
     PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(event));
 
-    if(updateHandleObj && PyCallable_Check(updateHandleObj))
-        result = PyEval_CallObject(updateHandleObj, pyobj_args);
+    result = PyEval_CallObject(updateHandleObj, pyobj_args);
+    if (!result) {
+        PyErr_Print();
+        PyErr_Clear();
+    }
 
     Py_XDECREF(result);
     Py_DECREF(pyobj_args);
@@ -1783,25 +1824,45 @@ libvirt_virEventUpdateHandleFunc(int fd, int event)
 
 
 static int
-libvirt_virEventRemoveHandleFunc(int fd)
+libvirt_virEventRemoveHandleFunc(int watch)
 {
-    PyObject *result = NULL;
+    PyObject *result;
     PyObject *pyobj_args;
+    PyObject *opaque;
+    PyObject *ff;
+    int retval = -1;
+    virFreeCallback cff;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
     pyobj_args = PyTuple_New(1);
-    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd));
+    PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(watch));
 
-    if(removeHandleObj && PyCallable_Check(removeHandleObj))
-        result = PyEval_CallObject(removeHandleObj, pyobj_args);
+    result = PyEval_CallObject(removeHandleObj, pyobj_args);
+    if (!result) {
+        PyErr_Print();
+        PyErr_Clear();
+    } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
+#if DEBUG_ERROR
+        printf("%s: %s must return opaque obj registered with %s"
+               "to avoid leaking libvirt memory\n",
+               __FUNCTION__, removeHandleName, addHandleName);
+#endif
+    } else {
+        opaque = PyTuple_GetItem(result, 1);
+        ff = PyTuple_GetItem(result, 2);
+        cff = PyvirFreeCallback_Get(ff);
+        if (cff)
+            (*cff)(PyvirVoidPtr_Get(opaque));
+        retval = 0;
+    }
 
     Py_XDECREF(result);
     Py_DECREF(pyobj_args);
 
     LIBVIRT_RELEASE_THREAD_STATE;
 
-    return 0;
+    return retval;
 }
 
 static int
@@ -1810,7 +1871,7 @@ libvirt_virEventAddTimeoutFunc(int timeout,
                                void *opaque,
                                virFreeCallback ff)
 {
-    PyObject *result = NULL;
+    PyObject *result;
 
     PyObject *python_cb;
 
@@ -1819,6 +1880,7 @@ libvirt_virEventAddTimeoutFunc(int timeout,
     PyObject *opaque_obj;
     PyObject *cb_args;
     PyObject *pyobj_args;
+    int retval = -1;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
@@ -1827,9 +1889,18 @@ libvirt_virEventAddTimeoutFunc(int timeout,
                                      "eventInvokeTimeoutCallback");
     if(!python_cb) {
 #if DEBUG_ERROR
-        printf("%s Error finding eventInvokeTimeoutCallback\n", __FUNCTION__);
+        printf("%s: Error finding eventInvokeTimeoutCallback\n", __FUNCTION__);
 #endif
         PyErr_Print();
+        PyErr_Clear();
+        goto cleanup;        
+    }
+    if (!PyCallable_Check(python_cb)) {
+#if DEBUG_ERROR
+        char *name = py_str(python_cb);
+        printf("%s: %s is not callable\n", __FUNCTION__, name);
+        free(name);
+#endif
         goto cleanup;
     }
     Py_INCREF(python_cb);
@@ -1850,15 +1921,24 @@ libvirt_virEventAddTimeoutFunc(int timeout,
     PyTuple_SetItem(pyobj_args, 1, python_cb);
     PyTuple_SetItem(pyobj_args, 2, cb_args);
 
-    if(addTimeoutObj && PyCallable_Check(addTimeoutObj))
-        result = PyEval_CallObject(addTimeoutObj, pyobj_args);
+    result = PyEval_CallObject(addTimeoutObj, pyobj_args);
+    if (!result) {
+        PyErr_Print();
+        PyErr_Clear();
+    } else if (!PyInt_Check(result)) {
+#if DEBUG_ERROR
+        printf("%s: %s should return an int\n", addTimeoutName, __FUNCTION__);
+#endif
+    } else {
+        retval = (int)PyInt_AsLong(result);
+    }
 
     Py_XDECREF(result);
     Py_DECREF(pyobj_args);
 
 cleanup:
     LIBVIRT_RELEASE_THREAD_STATE;
-    return 0;
+    return retval;
 }
 
 static void
@@ -1873,8 +1953,11 @@ libvirt_virEventUpdateTimeoutFunc(int timer, int timeout)
     PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timer));
     PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(timeout));
 
-    if(updateTimeoutObj && PyCallable_Check(updateTimeoutObj))
-        result = PyEval_CallObject(updateTimeoutObj, pyobj_args);
+    result = PyEval_CallObject(updateTimeoutObj, pyobj_args);
+    if (!result) {
+        PyErr_Print();
+        PyErr_Clear();
+    }
 
     Py_XDECREF(result);
     Py_DECREF(pyobj_args);
@@ -1887,45 +1970,85 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
 {
     PyObject *result = NULL;
     PyObject *pyobj_args;
+    PyObject *opaque;
+    PyObject *ff;
+    int retval = -1;
+    virFreeCallback cff;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
     pyobj_args = PyTuple_New(1);
     PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timer));
 
-    if(updateTimeoutObj && PyCallable_Check(updateTimeoutObj))
-        result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
+    result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
+    if (!result) {
+        PyErr_Print();
+        PyErr_Clear();
+    } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
+#if DEBUG_ERROR
+        printf("%s: %s must return opaque obj registered with %s"
+               "to avoid leaking libvirt memory\n",
+               __FUNCTION__, removeTimeoutName, addTimeoutName);
+#endif
+    } else {
+        opaque = PyTuple_GetItem(result, 1);
+        ff = PyTuple_GetItem(result, 2);
+        cff = PyvirFreeCallback_Get(ff);
+        if (cff)
+            (*cff)(PyvirVoidPtr_Get(opaque));
+        retval = 0;
+    }
 
     Py_XDECREF(result);
     Py_DECREF(pyobj_args);
 
     LIBVIRT_RELEASE_THREAD_STATE;
 
-    return 0;
+    return retval;
 }
 
 static PyObject *
 libvirt_virEventRegisterImpl(ATTRIBUTE_UNUSED PyObject * self,
                              PyObject * args)
 {
+    /* Unref the previously-registered impl (if any) */
     Py_XDECREF(addHandleObj);
+    free(addHandleName);
     Py_XDECREF(updateHandleObj);
+    free(updateHandleName);
     Py_XDECREF(removeHandleObj);
+    free(removeHandleName);
     Py_XDECREF(addTimeoutObj);
+    free(addTimeoutName);
     Py_XDECREF(updateTimeoutObj);
+    free(updateTimeoutName);
     Py_XDECREF(removeTimeoutObj);
-
-    if (!PyArg_ParseTuple
-        (args, (char *) "OOOOOO:virEventRegisterImpl",
-                        &addHandleObj,
-                        &updateHandleObj,
-                        &removeHandleObj,
-                        &addTimeoutObj,
-                        &updateTimeoutObj,
-                        &removeTimeoutObj
-        ))
+    free(removeTimeoutName);
+
+    /* Parse and check arguments */
+    if (!PyArg_ParseTuple(args, (char *) "OOOOOO:virEventRegisterImpl",
+                          &addHandleObj, &updateHandleObj,
+                          &removeHandleObj, &addTimeoutObj,
+                          &updateTimeoutObj, &removeTimeoutObj) ||
+        !PyCallable_Check(addHandleObj) ||
+        !PyCallable_Check(updateHandleObj) ||
+        !PyCallable_Check(removeHandleObj) ||
+        !PyCallable_Check(addTimeoutObj) ||
+        !PyCallable_Check(updateTimeoutObj) ||
+        !PyCallable_Check(removeTimeoutObj))
         return VIR_PY_INT_FAIL;
 
+    /* Get argument string representations (for error reporting) */
+    addHandleName = py_str(addTimeoutObj);
+    updateHandleName = py_str(updateHandleObj);
+    removeHandleName = py_str(removeHandleObj);
+    addTimeoutName = py_str(addTimeoutObj);
+    updateTimeoutName = py_str(updateTimeoutObj);
+    removeTimeoutName = py_str(removeTimeoutObj);
+
+    /* Inc refs since we're holding onto these objects until
+     * the next call (if any) to this function.
+     */
     Py_INCREF(addHandleObj);
     Py_INCREF(updateHandleObj);
     Py_INCREF(removeHandleObj);
@@ -1935,6 +2058,9 @@ libvirt_virEventRegisterImpl(ATTRIBUTE_UNUSED PyObject * self,
 
     LIBVIRT_BEGIN_ALLOW_THREADS;
 
+    /* Now register our C EventImpl, which will dispatch
+     * to the Python callbacks passed in as args.
+     */
     virEventRegisterImpl(libvirt_virEventAddHandleFunc,
                          libvirt_virEventUpdateHandleFunc,
                          libvirt_virEventRemoveHandleFunc,
diff --git a/python/libvir.py b/python/libvir.py
index 86bf422..b90f795 100644
--- a/python/libvir.py
+++ b/python/libvir.py
@@ -126,11 +126,11 @@ def getVersion (name = None):
 #
 # Invoke an EventHandle callback
 #
-def eventInvokeHandleCallback (fd, event, callback, opaque):
+def eventInvokeHandleCallback (watch, fd, event, callback, opaque):
     """
     Invoke the Event Impl Handle Callback in C
     """
-    libvirtmod.virEventInvokeHandleCallback(fd, event, callback, opaque);
+    libvirtmod.virEventInvokeHandleCallback(watch, fd, event, callback, opaque);
 
 #
 # Invoke an EventTimeout callback
diff --git a/python/libvirt_wrap.h b/python/libvirt_wrap.h
index b3cbcb8..7be9c1d 100644
--- a/python/libvirt_wrap.h
+++ b/python/libvirt_wrap.h
@@ -81,6 +81,14 @@ typedef struct {
     virEventTimeoutCallback obj;
 } PyvirEventTimeoutCallback_Object;
 
+#define PyvirFreeCallback_Get(v) (((v) == Py_None) ? NULL : \
+        (((PyvirFreeCallback_Object *)(v))->obj))
+
+typedef struct {
+    PyObject_HEAD
+    virFreeCallback obj;
+} PyvirFreeCallback_Object;
+
 #define PyvirVoidPtr_Get(v) (((v) == Py_None) ? NULL : \
         (((PyvirVoidPtr_Object *)(v))->obj))
 
diff --git a/python/types.c b/python/types.c
index 4285134..9530ad0 100644
--- a/python/types.c
+++ b/python/types.c
@@ -201,7 +201,6 @@ libvirt_virFreeCallbackWrap(virFreeCallback node)
     PyObject *ret;
 
     if (node == NULL) {
-        printf("%s: WARNING - Wrapping None\n", __FUNCTION__);
         Py_INCREF(Py_None);
         return (Py_None);
     }
diff --git a/python/virConnect.py b/python/virConnect.py
index ec29b33..1fdf548 100644
--- a/python/virConnect.py
+++ b/python/virConnect.py
@@ -32,12 +32,12 @@
             ret = libvirtmod.virConnectDomainEventRegister(self._o, self)
             if ret == -1: raise libvirtError ('virConnectDomainEventRegister() failed', conn=self)
 
-    def dispatchDomainEventCallbacks(self, dom, event):
+    def dispatchDomainEventCallbacks(self, dom, event, detail):
         """Dispatches events to python user domain event callbacks
         """
         try:
             for cb,opaque in self.domainEventCallbacks.items():
-                cb(self,dom,event,opaque)
+                cb(self,dom,event,detail,opaque)
             return 0
         except AttributeError:
             pass

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]