[libvirt] [PATCH] fix python events
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(-)
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.
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.
David Lively <dlively@virtualiron.com> wrote: ... Hi David, No big deal, but there are several debug printf uses here that look like they try to print NULL pointers upon memory allocation failure. It's ok with glibc's printf of course, but not for others.
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/python/libvir.c b/python/libvir.c ... @@ -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);
Please handle name == NULL. Otherwise, with DEBUG_ERROR, the printf will segfault on most non-glibc-based systems. E.g., define this, then wrap the arg with NN(...). static inline char *NN (const char *s) { return s ? s : (char *) "<NULL>"; } printf("%s: %s is not callable\n", __FUNCTION__, NN(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__);
Same here, addHandleName can be NULL.
+#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);
Same here, for both.
+#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);
Same here.
+ 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__);
addTimeoutName can be NULL.
+#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 ... @@ -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);
Likewise.
+#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); ...
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote:
No big deal, but there are several debug printf uses here that look like they try to print NULL pointers upon memory allocation failure. It's ok with glibc's printf of course, but not for others.
You're right. Attached patch fixes those issues. It also fixes some cases in which I got some printf string operands switched around ... I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name). Dave examples/domain-events/events-python/event-test.py | 29 +-- python/libvir.c | 203 +++++++++++++++++---- python/libvir.py | 4 python/libvirt_wrap.h | 8 python/types.c | 1 python/virConnect.py | 4 6 files changed, 197 insertions(+), 52 deletions(-)
David Lively <dlively@virtualiron.com> wrote:
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote:
No big deal, but there are several debug printf uses here that look like they try to print NULL pointers upon memory allocation failure. It's ok with glibc's printf of course, but not for others.
You're right. Attached patch fixes those issues. It also fixes some cases in which I got some printf string operands switched around ...
I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name).
That's better, indeed. I prefer your NAME macro, too. ACK, modulo syntax:
diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py @@ -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])
Reversed diff? Better to split long lines, not to join.
#print "DEBUG EXIT" #break diff --git a/python/libvir.c b/python/libvir.c index 7d58442..fed1031 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); +} + <<<
Trailing white space ^^.
On Sat, 2008-11-22 at 08:21 +0100, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote: I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name).
That's better, indeed. I prefer your NAME macro, too. ACK, modulo syntax:
...
Reversed diff? Better to split long lines, not to join.
Ok. Fixed in attached patch.
+ <<<
Trailing white space ^^.
.. along with this one ... and the trailing whitespace on line 1982. *This* time I ran syntax-check :-) Dave examples/domain-events/events-python/event-test.py | 26 +- python/libvir.c | 203 +++++++++++++++++---- python/libvir.py | 4 python/libvirt_wrap.h | 8 python/types.c | 1 python/virConnect.py | 4 6 files changed, 196 insertions(+), 50 deletions(-)
David Lively <dlively@virtualiron.com> wrote:
On Sat, 2008-11-22 at 08:21 +0100, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote: I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name).
That's better, indeed. I prefer your NAME macro, too. ACK, modulo syntax:
...
Reversed diff? Better to split long lines, not to join.
Ok. Fixed in attached patch.
+ <<<
Trailing white space ^^.
.. along with this one ... and the trailing whitespace on line 1982.
*This* time I ran syntax-check :-)
;-) Unqualified ACK, then, assuming those are the only changes.
On Mon, 2008-11-24 at 17:30 +0100, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
On Sat, 2008-11-22 at 08:21 +0100, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote: I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name).
That's better, indeed. I prefer your NAME macro, too. ACK, modulo syntax:
...
Reversed diff? Better to split long lines, not to join.
Ok. Fixed in attached patch.
+ <<<
Trailing white space ^^.
.. along with this one ... and the trailing whitespace on line 1982.
*This* time I ran syntax-check :-)
;-) Unqualified ACK, then, assuming those are the only changes.
Those are the only changes. Dave
On Mon, Nov 24, 2008 at 11:44:15AM -0500, David Lively wrote:
On Mon, 2008-11-24 at 17:30 +0100, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
On Sat, 2008-11-22 at 08:21 +0100, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote: I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name).
That's better, indeed. I prefer your NAME macro, too. ACK, modulo syntax:
...
Reversed diff? Better to split long lines, not to join.
Ok. Fixed in attached patch.
+ <<<
Trailing white space ^^.
.. along with this one ... and the trailing whitespace on line 1982.
*This* time I ran syntax-check :-)
;-) Unqualified ACK, then, assuming those are the only changes.
Those are the only changes.
Ok, I committed this patch now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Nov 21, 2008 at 02:14:57PM -0500, David Lively wrote:
On Fri, 2008-11-21 at 17:51 +0100, Jim Meyering wrote:
No big deal, but there are several debug printf uses here that look like they try to print NULL pointers upon memory allocation failure. It's ok with glibc's printf of course, but not for others.
You're right. Attached patch fixes those issues. It also fixes some cases in which I got some printf string operands switched around ...
I'm printing the (user-supplied) object names to help in debugging misbehaving python EventImpls (since there's no static type checking to catch these kinds of things). So instead of printing "<NULL>" when we can't alloc the name, I'm printing something a little more helpful (the appropriate "generic" name).
ACK. This looks ok to me now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
David Lively -
Jim Meyering