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(-)
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 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);
+}
+
+
/************************************************************************
* *
* 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))
@@ -1780,12 +1793,19 @@ 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;
+#define NAME(fn) ( fn ## Name ? fn ## Name : # fn )
static int
libvirt_virEventAddHandleFunc (int fd,
@@ -1794,13 +1814,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;
@@ -1809,9 +1830,19 @@ 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 ? name : "libvirt.eventInvokeHandleCallback");
+ free(name);
+#endif
goto cleanup;
}
Py_INCREF(python_cb);
@@ -1832,8 +1863,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", __FUNCTION__, NAME(addHandle));
+#endif
+ } else {
+ retval = (int)PyInt_AsLong(result);
+ }
Py_XDECREF(result);
Py_DECREF(pyobj_args);
@@ -1841,23 +1881,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);
@@ -1867,25 +1910,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__, NAME(removeHandle), NAME(addHandle));
+#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
@@ -1894,7 +1957,7 @@ libvirt_virEventAddTimeoutFunc(int timeout,
void *opaque,
virFreeCallback ff)
{
- PyObject *result = NULL;
+ PyObject *result;
PyObject *python_cb;
@@ -1903,6 +1966,7 @@ libvirt_virEventAddTimeoutFunc(int timeout,
PyObject *opaque_obj;
PyObject *cb_args;
PyObject *pyobj_args;
+ int retval = -1;
LIBVIRT_ENSURE_THREAD_STATE;
@@ -1911,9 +1975,19 @@ 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 ? name : "libvirt.eventInvokeTimeoutCallback");
+ free(name);
+#endif
goto cleanup;
}
Py_INCREF(python_cb);
@@ -1934,15 +2008,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", __FUNCTION__, NAME(addTimeout));
+#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
@@ -1957,8 +2040,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);
@@ -1971,45 +2057,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__, NAME(removeTimeout), NAME(addTimeout));
+#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);
@@ -2019,6 +2145,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 9bcfc96..fa773d2 100644
--- a/python/libvirt_wrap.h
+++ b/python/libvirt_wrap.h
@@ -91,6 +91,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 5c4e6ad..c773f30 100644
--- a/python/types.c
+++ b/python/types.c
@@ -216,7 +216,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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list