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

Re: [libvirt] PATCH: Misc fixes to events code



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;
>  
>  /**
> 
> 


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