These look good to me +1 Daniel P. Berrange wrote on 11/04/2008 02:56 PM: > I'm trying out the new events code with virt-manager and stumbled on some > minor bugs > > - The python binding forgot to call LIBVIRT_BEGIN_ALLOW_THREADS, > LIBVIRT_END_ALLOW_THREADS, LIBVIRT_ENSURE_THREAD_STATE or > LIBVIRT_RELEASE_THREAD_STATE which meant virt-manager crashed > and burned in a heap of python interpretor state corruption [1] > > - The API docs extracted assumes that enums without explicit value > assignements started from 1 instead of 0. This caused an off-by-1 > error for all the VIR_DOMAIN_EVENT_* constants in the generated python > code. > > I briefly toyed with fixing docs/apibuild.py enum handling but then instead > decided to just change libvirt.h to give explicit values. > > With these two fixes and some hacks to virt-manager, I've got the > events triggering display state refreshes. > > A couple more things I've not fixed but are on the todo list > > - THe QEMU driver doesn't emit the ADDED or REMOVED events for VMs > which virt-manager requires in order to fully avoid polling. > - The VIR_EVENT_* constants aren't included in python, because the > API docs can't cope with enum value asignments looking like > (1 << 0) (ie fails to parse the left-shifts) > > Regards, > Daniel > > [1] FYI, i'm using the libvirt-glib python binding in virt-manager > http://libvirt.org/hg/libvirt-glib/ which is slightly bad too > by not doing glib thread locking calls, but we're lucky because > libvirt.so doesn't invoke any glib calls itself from the event > callback context. > > > Index: python/libvir.c > =================================================================== > RCS file: /data/cvs/libvirt/python/libvir.c,v > retrieving revision 1.43 > diff -u -p -r1.43 libvir.c > --- python/libvir.c 31 Oct 2008 10:13:45 -0000 1.43 > +++ python/libvir.c 4 Nov 2008 19:45:42 -0000 > @@ -1537,29 +1537,33 @@ libvirt_virConnectDomainEventCallback(vi > PyObject *pyobj_ret; > > PyObject *pyobj_conn_inst = (PyObject*)opaque; > - PyObject *pyobj_dom = libvirt_virDomainPtrWrap(dom); > + PyObject *pyobj_dom; > > PyObject *pyobj_dom_args; > PyObject *pyobj_dom_inst; > > PyObject *dom_class; > + int ret = -1; > + > + LIBVIRT_ENSURE_THREAD_STATE; > > /* Create a python instance of this virDomainPtr */ > + pyobj_dom = libvirt_virDomainPtrWrap(dom); > pyobj_dom_args = PyTuple_New(2); > if(PyTuple_SetItem(pyobj_dom_args, 0, pyobj_conn_inst)!=0) { > printf("%s error creating tuple",__FUNCTION__); > - return -1; > + goto cleanup; > } > if(PyTuple_SetItem(pyobj_dom_args, 1, pyobj_dom)!=0) { > printf("%s error creating tuple",__FUNCTION__); > - return -1; > + goto cleanup; > } > Py_INCREF(pyobj_conn_inst); > > dom_class = getLibvirtDomainClassObject(); > if(!PyClass_Check(dom_class)) { > printf("%s dom_class is not a class!\n", __FUNCTION__); > - return -1; > + goto cleanup; > } > > pyobj_dom_inst = PyInstance_New(dom_class, > @@ -1571,7 +1575,7 @@ libvirt_virConnectDomainEventCallback(vi > if(!pyobj_dom_inst) { > printf("%s Error creating a python instance of virDomain\n", __FUNCTION__); > PyErr_Print(); > - return -1; > + goto cleanup; > } > > /* Call the Callback Dispatcher */ > @@ -1586,12 +1590,15 @@ libvirt_virConnectDomainEventCallback(vi > if(!pyobj_ret) { > printf("%s - ret:%p\n", __FUNCTION__, pyobj_ret); > PyErr_Print(); > - return -1; > } else { > Py_DECREF(pyobj_ret); > - return 0; > + ret = 0; > } > > + > +cleanup: > + LIBVIRT_RELEASE_THREAD_STATE; > + return -1; > } > > static PyObject * > @@ -1620,10 +1627,14 @@ libvirt_virConnectDomainEventRegister(AT > > Py_INCREF(pyobj_conn_inst); > > + LIBVIRT_BEGIN_ALLOW_THREADS; > + > ret = virConnectDomainEventRegister(conn, > libvirt_virConnectDomainEventCallback, > (void *)pyobj_conn_inst); > > + LIBVIRT_END_ALLOW_THREADS; > + > py_retval = libvirt_intWrap(ret); > return (py_retval); > } > @@ -1650,8 +1661,12 @@ libvirt_virConnectDomainEventDeregister( > > conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); > > + LIBVIRT_BEGIN_ALLOW_THREADS; > + > ret = virConnectDomainEventDeregister(conn, libvirt_virConnectDomainEventCallback); > > + LIBVIRT_END_ALLOW_THREADS; > + > Py_DECREF(pyobj_conn_inst); > py_retval = libvirt_intWrap(ret); > return (py_retval); > @@ -1679,13 +1694,15 @@ libvirt_virEventAddHandleFunc (int fd A > PyObject *cb_args; > PyObject *pyobj_args; > > + LIBVIRT_ENSURE_THREAD_STATE; > + > /* Lookup the python callback */ > python_cb = PyDict_GetItemString(getLibvirtDictObject(), > "eventInvokeHandleCallback"); > if(!python_cb) { > printf("%s Error finding eventInvokeHandleCallback\n", __FUNCTION__); > PyErr_Print(); > - return -1; > + goto cleanup; > } > Py_INCREF(python_cb); > > @@ -1708,6 +1725,10 @@ libvirt_virEventAddHandleFunc (int fd A > > Py_XDECREF(result); > Py_DECREF(pyobj_args); > + > +cleanup: > + LIBVIRT_RELEASE_THREAD_STATE; > + > return 0; > } > > @@ -1715,7 +1736,11 @@ static void > libvirt_virEventUpdateHandleFunc(int fd, int event) > { > PyObject *result = NULL; > - PyObject *pyobj_args = PyTuple_New(2); > + PyObject *pyobj_args; > + > + LIBVIRT_ENSURE_THREAD_STATE; > + > + pyobj_args = PyTuple_New(2); > PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd)); > PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(event)); > > @@ -1724,13 +1749,20 @@ libvirt_virEventUpdateHandleFunc(int fd, > > Py_XDECREF(result); > Py_DECREF(pyobj_args); > + > + LIBVIRT_RELEASE_THREAD_STATE; > } > > + > static int > libvirt_virEventRemoveHandleFunc(int fd) > { > PyObject *result = NULL; > - PyObject *pyobj_args = PyTuple_New(1); > + PyObject *pyobj_args; > + > + LIBVIRT_ENSURE_THREAD_STATE; > + > + pyobj_args = PyTuple_New(1); > PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd)); > > if(removeHandleObj && PyCallable_Check(removeHandleObj)) > @@ -1738,6 +1770,9 @@ libvirt_virEventRemoveHandleFunc(int fd) > > Py_XDECREF(result); > Py_DECREF(pyobj_args); > + > + LIBVIRT_RELEASE_THREAD_STATE; > + > return 0; > } > > @@ -1754,13 +1789,15 @@ libvirt_virEventAddTimeoutFunc(int timeo > PyObject *cb_args; > PyObject *pyobj_args; > > + LIBVIRT_ENSURE_THREAD_STATE; > + > /* Lookup the python callback */ > python_cb = PyDict_GetItemString(getLibvirtDictObject(), > "eventInvokeTimeoutCallback"); > if(!python_cb) { > printf("%s Error finding eventInvokeTimeoutCallback\n", __FUNCTION__); > PyErr_Print(); > - return -1; > + goto cleanup; > } > Py_INCREF(python_cb); > > @@ -1783,6 +1820,9 @@ libvirt_virEventAddTimeoutFunc(int timeo > > Py_XDECREF(result); > Py_DECREF(pyobj_args); > + > +cleanup: > + LIBVIRT_RELEASE_THREAD_STATE; > return 0; > } > > @@ -1790,7 +1830,11 @@ static void > libvirt_virEventUpdateTimeoutFunc(int timer, int timeout) > { > PyObject *result = NULL; > - PyObject *pyobj_args = PyTuple_New(2); > + PyObject *pyobj_args; > + > + LIBVIRT_ENSURE_THREAD_STATE; > + > + pyobj_args = PyTuple_New(2); > PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timer)); > PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(timeout)); > > @@ -1799,13 +1843,19 @@ libvirt_virEventUpdateTimeoutFunc(int ti > > Py_XDECREF(result); > Py_DECREF(pyobj_args); > + > + LIBVIRT_RELEASE_THREAD_STATE; > } > > static int > libvirt_virEventRemoveTimeoutFunc(int timer) > { > PyObject *result = NULL; > - PyObject *pyobj_args = PyTuple_New(1); > + PyObject *pyobj_args; > + > + LIBVIRT_ENSURE_THREAD_STATE; > + > + pyobj_args = PyTuple_New(1); > PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timer)); > > if(updateTimeoutObj && PyCallable_Check(updateTimeoutObj)) > @@ -1813,6 +1863,9 @@ libvirt_virEventRemoveTimeoutFunc(int ti > > Py_XDECREF(result); > Py_DECREF(pyobj_args); > + > + LIBVIRT_RELEASE_THREAD_STATE; > + > return 0; > } > > @@ -1845,6 +1898,8 @@ libvirt_virEventRegisterImpl(ATTRIBUTE_U > Py_INCREF(updateTimeoutObj); > Py_INCREF(removeTimeoutObj); > > + LIBVIRT_BEGIN_ALLOW_THREADS; > + > virEventRegisterImpl(libvirt_virEventAddHandleFunc, > libvirt_virEventUpdateHandleFunc, > libvirt_virEventRemoveHandleFunc, > @@ -1852,6 +1907,8 @@ libvirt_virEventRegisterImpl(ATTRIBUTE_U > libvirt_virEventUpdateTimeoutFunc, > libvirt_virEventRemoveTimeoutFunc); > > + LIBVIRT_END_ALLOW_THREADS; > + > return VIR_PY_INT_SUCCESS; > } > > Index: include/libvirt/libvirt.h.in > =================================================================== > RCS file: /data/cvs/libvirt/include/libvirt/libvirt.h.in,v > retrieving revision 1.57 > diff -u -p -r1.57 libvirt.h.in > --- include/libvirt/libvirt.h.in 24 Oct 2008 12:05:39 -0000 1.57 > +++ include/libvirt/libvirt.h.in 4 Nov 2008 19:45:42 -0000 > @@ -1004,14 +1004,14 @@ virDomainPtr virDomainCreateL > * a virDomainEventType is emitted during domain lifecycle events > */ > typedef enum { > - VIR_DOMAIN_EVENT_ADDED, > - VIR_DOMAIN_EVENT_REMOVED, > - VIR_DOMAIN_EVENT_STARTED, > - VIR_DOMAIN_EVENT_SUSPENDED, > - VIR_DOMAIN_EVENT_RESUMED, > - VIR_DOMAIN_EVENT_STOPPED, > - VIR_DOMAIN_EVENT_SAVED, > - VIR_DOMAIN_EVENT_RESTORED, > + VIR_DOMAIN_EVENT_ADDED = 0, > + VIR_DOMAIN_EVENT_REMOVED = 1, > + VIR_DOMAIN_EVENT_STARTED = 2, > + VIR_DOMAIN_EVENT_SUSPENDED = 3, > + VIR_DOMAIN_EVENT_RESUMED = 4, > + VIR_DOMAIN_EVENT_STOPPED = 5, > + VIR_DOMAIN_EVENT_SAVED = 6, > + VIR_DOMAIN_EVENT_RESTORED = 7, > } virDomainEventType; > > /** > > -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list