[libvirt] [PATCH 2/4] daemon: call free callbacks synchronously in event loop impl

[Nikolay Shirokovskiy]

Default event loop impl delays deleting registered handles and
timeouts so that deleting is safe from event handlers. But
this means deletions after event loop is finished will never
occur and particularly assosiated callback objects will not
be freed. For this reason network clients that exist at
the moment of libvirtd daemon exit are never get disposed
and daemon object itself too if there are admin server clients.

Let's call free callbacks immediately we don't need to delay
this operation, only removing handles from the list.

This breaks virnetsocket.c immediately as socket is locked
in freeing callback and callback is called synchronously
from virNetSocketRemoveIOCallback which holds the lock already.
So fix it to pass intermediate callback object to the loop
instead of socket itself. I've checked other users of
virEventAddHandle, looks like they don't have such problems.
---
 src/rpc/virnetsocket.c  | 70 ++++++++++++++++++++++---------------------------
 src/util/vireventpoll.c | 24 +++++++----------
 2 files changed, 42 insertions(+), 52 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 405f5ba..732a993 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -69,6 +69,16 @@
 
 VIR_LOG_INIT("rpc.netsocket");
 
+struct _virNetSocketCallbackObject {
+    virNetSocketPtr sock;
+    virNetSocketIOFunc func;
+    void *opaque;
+    virFreeCallback ff;
+};
+
+typedef struct _virNetSocketCallbackObject virNetSocketCallbackObject;
+typedef virNetSocketCallbackObject *virNetSocketCallbackObjectPtr;
+
 struct _virNetSocket {
     virObjectLockable parent;
 
@@ -78,11 +88,6 @@ struct _virNetSocket {
     int errfd;
     bool client;
 
-    /* Event callback fields */
-    virNetSocketIOFunc func;
-    void *opaque;
-    virFreeCallback ff;
-
     virSocketAddr localAddr;
     virSocketAddr remoteAddr;
     char *localAddrStrSASL;
@@ -1928,38 +1933,19 @@ static void virNetSocketEventHandle(int watch ATTRIBUTE_UNUSED,
                                     int events,
                                     void *opaque)
 {
-    virNetSocketPtr sock = opaque;
-    virNetSocketIOFunc func;
-    void *eopaque;
-
-    virObjectLock(sock);
-    func = sock->func;
-    eopaque = sock->opaque;
-    virObjectUnlock(sock);
-
-    if (func)
-        func(sock, events, eopaque);
+    virNetSocketCallbackObjectPtr o = opaque;
+    if (o->func)
+        o->func(o->sock, events, o->opaque);
 }
 
 
 static void virNetSocketEventFree(void *opaque)
 {
-    virNetSocketPtr sock = opaque;
-    virFreeCallback ff;
-    void *eopaque;
-
-    virObjectLock(sock);
-    ff = sock->ff;
-    eopaque = sock->opaque;
-    sock->func = NULL;
-    sock->ff = NULL;
-    sock->opaque = NULL;
-    virObjectUnlock(sock);
-
-    if (ff)
-        ff(eopaque);
-
-    virObjectUnref(sock);
+    virNetSocketCallbackObjectPtr o = opaque;
+    if (o->ff)
+        o->ff(o->opaque);
+    virObjectUnref(o->sock);
+    VIR_FREE(o);
 }
 
 int virNetSocketAddIOCallback(virNetSocketPtr sock,
@@ -1969,32 +1955,40 @@ int virNetSocketAddIOCallback(virNetSocketPtr sock,
                               virFreeCallback ff)
 {
     int ret = -1;
+    virNetSocketCallbackObjectPtr cbobj = NULL;
 
-    virObjectRef(sock);
     virObjectLock(sock);
     if (sock->watch >= 0) {
         VIR_DEBUG("Watch already registered on socket %p", sock);
         goto cleanup;
     }
 
+    if (VIR_ALLOC(cbobj) < 0)
+        goto cleanup;
+
+    cbobj->sock = virObjectRef(sock);
+    cbobj->func = func;
+    cbobj->opaque = opaque;
+    cbobj->ff = ff;
+
     if ((sock->watch = virEventAddHandle(sock->fd,
                                          events,
                                          virNetSocketEventHandle,
-                                         sock,
+                                         cbobj,
                                          virNetSocketEventFree)) < 0) {
         VIR_DEBUG("Failed to register watch on socket %p", sock);
         goto cleanup;
     }
-    sock->func = func;
-    sock->opaque = opaque;
-    sock->ff = ff;
 
     ret = 0;
 
  cleanup:
     virObjectUnlock(sock);
-    if (ret != 0)
+    if (ret != 0) {
+        VIR_FREE(cbobj);
         virObjectUnref(sock);
+    }
+
     return ret;
 }
 
diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 81ecab4..e152d23 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -198,6 +198,11 @@ int virEventPollRemoveHandle(int watch)
         if (eventLoop.handles[i].watch == watch) {
             EVENT_DEBUG("mark delete %zu %d", i, eventLoop.handles[i].fd);
             eventLoop.handles[i].deleted = 1;
+            if (eventLoop.handles[i].ff) {
+                virMutexUnlock(&eventLoop.lock);
+                eventLoop.handles[i].ff(eventLoop.handles[i].opaque);
+                virMutexLock(&eventLoop.lock);
+            }
             virEventPollInterruptLocked();
             virMutexUnlock(&eventLoop.lock);
             return 0;
@@ -315,6 +320,11 @@ int virEventPollRemoveTimeout(int timer)
             continue;
 
         if (eventLoop.timeouts[i].timer == timer) {
+            if (eventLoop.timeouts[i].ff) {
+                virMutexUnlock(&eventLoop.lock);
+                eventLoop.timeouts[i].ff(eventLoop.timeouts[i].opaque);
+                virMutexLock(&eventLoop.lock);
+            }
             eventLoop.timeouts[i].deleted = 1;
             virEventPollInterruptLocked();
             virMutexUnlock(&eventLoop.lock);
@@ -536,13 +546,6 @@ static void virEventPollCleanupTimeouts(void)
         PROBE(EVENT_POLL_PURGE_TIMEOUT,
               "timer=%d",
               eventLoop.timeouts[i].timer);
-        if (eventLoop.timeouts[i].ff) {
-            virFreeCallback ff = eventLoop.timeouts[i].ff;
-            void *opaque = eventLoop.timeouts[i].opaque;
-            virMutexUnlock(&eventLoop.lock);
-            ff(opaque);
-            virMutexLock(&eventLoop.lock);
-        }
 
         if ((i+1) < eventLoop.timeoutsCount) {
             memmove(eventLoop.timeouts+i,
@@ -585,13 +588,6 @@ static void virEventPollCleanupHandles(void)
         PROBE(EVENT_POLL_PURGE_HANDLE,
               "watch=%d",
               eventLoop.handles[i].watch);
-        if (eventLoop.handles[i].ff) {
-            virFreeCallback ff = eventLoop.handles[i].ff;
-            void *opaque = eventLoop.handles[i].opaque;
-            virMutexUnlock(&eventLoop.lock);
-            ff(opaque);
-            virMutexLock(&eventLoop.lock);
-        }
 
         if ((i+1) < eventLoop.handlesCount) {
             memmove(eventLoop.handles+i,
-- 
1.8.3.1