Allow for ff callbacks to be called by custom event implementations

The documentation says:
> If the opaque user data requires free'ing when the handle is
> unregistered, then a 2nd callback can be supplied for this purpose.
> This callback needs to be invoked from a clean stack. If 'ff'
> callbacks are invoked directly from the virEventRemoveHandleFunc they
> will likely deadlock in libvirt.

And they did deadlock. In removeTimeout too. Now we supply a custom
function to pick it from the opaque blob and fire.

Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

libvirt-override.c

@@ -5223,6 +5223,9 @@ libvirt_virEventAddHandleFunc(int fd,
 
     VIR_PY_TUPLE_SET_GOTO(pyobj_args, 3, cb_args, cleanup);
 
+    /* If changing contents of the opaque object, please also change
+     * virEventInvokeFreeCallback() in libvirt-override.py
+     */
     VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventHandleCallbackWrap(cb), cleanup);
     VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
     VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), cleanup);
@@ -5279,10 +5282,7 @@ libvirt_virEventRemoveHandleFunc(int watch)
 {
     PyObject *result = NULL;
     PyObject *pyobj_args;
-    PyObject *opaque;
-    PyObject *ff;
     int retval = -1;
-    virFreeCallback cff;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
@@ -5292,20 +5292,11 @@ libvirt_virEventRemoveHandleFunc(int watch)
     VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(watch), cleanup);
 
     result = PyEval_CallObject(removeHandleObj, pyobj_args);
-    if (!result) {
+    if (result) {
+        retval = 0;
+    } else {
         PyErr_Print();
         PyErr_Clear();
-    } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-        DEBUG("%s: %s must return opaque obj registered with %s"
-              "to avoid leaking libvirt memory\n",
-              __FUNCTION__, NAME(removeHandle), NAME(addHandle));
-    } else {
-        opaque = PyTuple_GetItem(result, 1);
-        ff = PyTuple_GetItem(result, 2);
-        cff = PyvirFreeCallback_Get(ff);
-        if (cff)
-            (*cff)(PyvirVoidPtr_Get(opaque));
-        retval = 0;
     }
 
  cleanup:
@@ -5350,6 +5341,9 @@ libvirt_virEventAddTimeoutFunc(int timeout,
 
     VIR_PY_TUPLE_SET_GOTO(pyobj_args, 2, cb_args, cleanup);
 
+    /* If changing contents of the opaque object, please also change
+     * virEventInvokeFreeCallback() in libvirt-override.py
+     */
     VIR_PY_TUPLE_SET_GOTO(cb_args, 0, libvirt_virEventTimeoutCallbackWrap(cb), cleanup);
     VIR_PY_TUPLE_SET_GOTO(cb_args, 1, libvirt_virVoidPtrWrap(opaque), cleanup);
     VIR_PY_TUPLE_SET_GOTO(cb_args, 2, libvirt_virFreeCallbackWrap(ff), cleanup);
@@ -5403,10 +5397,7 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
 {
     PyObject *result = NULL;
     PyObject *pyobj_args;
-    PyObject *opaque;
-    PyObject *ff;
     int retval = -1;
-    virFreeCallback cff;
 
     LIBVIRT_ENSURE_THREAD_STATE;
 
@@ -5416,20 +5407,11 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
     VIR_PY_TUPLE_SET_GOTO(pyobj_args, 0, libvirt_intWrap(timer), cleanup);
 
     result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
-    if (!result) {
+    if (result) {
+        retval = 0;
+    } else {
         PyErr_Print();
         PyErr_Clear();
-    } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
-        DEBUG("%s: %s must return opaque obj registered with %s"
-              "to avoid leaking libvirt memory\n",
-              __FUNCTION__, NAME(removeTimeout), NAME(addTimeout));
-    } else {
-        opaque = PyTuple_GetItem(result, 1);
-        ff = PyTuple_GetItem(result, 2);
-        cff = PyvirFreeCallback_Get(ff);
-        if (cff)
-            (*cff)(PyvirVoidPtr_Get(opaque));
-        retval = 0;
     }
 
  cleanup:
@@ -5558,6 +5540,31 @@ libvirt_virEventInvokeTimeoutCallback(PyObject *self ATTRIBUTE_UNUSED,
     return VIR_PY_INT_SUCCESS;
 }
 
+static PyObject *
+libvirt_virEventInvokeFreeCallback(PyObject *self ATTRIBUTE_UNUSED,
+                                   PyObject *args)
+{
+    PyObject *py_f;
+    PyObject *py_opaque;
+    virFreeCallback cb;
+    void *opaque;
+
+    if (!PyArg_ParseTuple(args, (char *) "OO:virEventInvokeFreeCallback",
+                          &py_f, &py_opaque))
+        return NULL;
+
+    cb     = (virFreeCallback) PyvirEventHandleCallback_Get(py_f);
+    opaque = (void *) PyvirVoidPtr_Get(py_opaque);
+
+    if (cb) {
+        LIBVIRT_BEGIN_ALLOW_THREADS;
+        cb(opaque);
+        LIBVIRT_END_ALLOW_THREADS;
+    }
+
+    return VIR_PY_INT_SUCCESS;
+}
+
 static void
 libvirt_virEventHandleCallback(int watch,
                                int fd,
@@ -9572,6 +9579,7 @@ static PyMethodDef libvirtMethods[] = {
     {(char *) "virEventAddTimeout", libvirt_virEventAddTimeout, METH_VARARGS, NULL},
     {(char *) "virEventInvokeHandleCallback", libvirt_virEventInvokeHandleCallback, METH_VARARGS, NULL},
     {(char *) "virEventInvokeTimeoutCallback", libvirt_virEventInvokeTimeoutCallback, METH_VARARGS, NULL},
+    {(char *) "virEventInvokeFreeCallback", libvirt_virEventInvokeFreeCallback, METH_VARARGS, NULL},
     {(char *) "virNodeListDevices", libvirt_virNodeListDevices, METH_VARARGS, NULL},
 #if LIBVIR_CHECK_VERSION(0, 10, 2)
     {(char *) "virConnectListAllNodeDevices", libvirt_virConnectListAllNodeDevices, METH_VARARGS, NULL},

libvirt-override.py

@@ -211,3 +211,26 @@ def virEventAddTimeout(timeout, cb, opaque):
     ret = libvirtmod.virEventAddTimeout(timeout, cbData)
     if ret == -1: raise libvirtError ('virEventAddTimeout() failed')
     return ret
+
+
+#
+# a caller for the ff callbacks for custom event loop implementations
+#
+
+def virEventInvokeFreeCallback(opaque):
+    """
+    Execute callback which frees the opaque buffer
+
+    @opaque: the opaque object passed to addHandle or addTimeout
+
+    WARNING: This function should not be called from any call by libvirt's
+    core. It will most probably cause deadlock in C-level libvirt code.
+    Instead it should be scheduled and called from implementation's stack.
+
+    See https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFunc
+    for more information.
+
+    This function is not dependent on any event loop implementation.
+    """
+
+    libvirtmod.virEventInvokeFreeCallback(opaque[2], opaque[1])

sanitytest.py

@@ -349,7 +349,8 @@ for klass in gotfunctions:
         continue
     for func in sorted(gotfunctions[klass]):
         # These are pure python methods with no C APi
-        if func in ["connect", "getConnect", "domain", "getDomain"]:
+        if func in ["connect", "getConnect", "domain", "getDomain",
+                    "virEventInvokeFreeCallback"]:
             continue
 
         key = "%s.%s" % (klass, func)