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

[libvirt] [PATCH 8/8] Only add the timer when a callback is registered



From: "Daniel P. Berrange" <berrange redhat com>

The lifetime of the virDomainEventState object is tied to
the lifetime of the driver, which in stateless drivers is
tied to the lifetime of the virConnectPtr.

If we add & remove a timer when allocating/freeing the
virDomainEventState object, we can get a situation where
the timer still triggers once after virDomainEventState
has been freed. The timeout callback can't keep a ref
on the event state though, since that would be a circular
reference.

The trick is to only register the timer when a callback
is registered with the event state & remove the timer
when the callback is unregistered.

The demo for the bug is to run

  while true ; do date ; ../tools/virsh -q -c test:///default 'shutdown test; undefine test; dominfo test' ; done

prior to this fix, it will frequently hang and / or
crash, or corrupt memory
---
 src/conf/domain_event.c    |   87 ++++++++++++++++++++++++++++++++------------
 src/conf/domain_event.h    |    2 +-
 src/libxl/libxl_driver.c   |    2 +-
 src/lxc/lxc_driver.c       |    2 +-
 src/qemu/qemu_driver.c     |    2 +-
 src/remote/remote_driver.c |    2 +-
 src/test/test_driver.c     |    2 +-
 src/uml/uml_driver.c       |    2 +-
 src/vbox/vbox_tmpl.c       |    2 +-
 src/xen/xen_driver.c       |    2 +-
 10 files changed, 72 insertions(+), 33 deletions(-)

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index da99d9f..26b4967 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -631,13 +631,9 @@ static void virDomainEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque)
 
 /**
  * virDomainEventStateNew:
- * @requireTimer: If true, return an error if registering the timer fails.
- *                This is fatal for drivers that sit behind the daemon
- *                (qemu, lxc), since there should always be a event impl
- *                registered.
  */
 virDomainEventStatePtr
-virDomainEventStateNew(bool requireTimer)
+virDomainEventStateNew(void)
 {
     virDomainEventStatePtr state = NULL;
 
@@ -658,23 +654,10 @@ virDomainEventStateNew(bool requireTimer)
         goto error;
     }
 
-    if (!(state->queue = virDomainEventQueueNew())) {
+    if (!(state->queue = virDomainEventQueueNew()))
         goto error;
-    }
 
-    if ((state->timer = virEventAddTimeout(-1,
-                                           virDomainEventTimer,
-                                           state,
-                                           NULL)) < 0) {
-        if (requireTimer == false) {
-            VIR_DEBUG("virEventAddTimeout failed: No addTimeoutImpl defined. "
-                      "continuing without events.");
-        } else {
-            eventReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                             _("could not initialize domain event timer"));
-            goto error;
-        }
-    }
+    state->timer = -1;
 
     return state;
 
@@ -1311,7 +1294,6 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
     state->queue->count = 0;
     state->queue->events = NULL;
     virEventUpdateTimeout(state->timer, -1);
-    virDomainEventStateUnlock(state);
 
     virDomainEventQueueDispatch(&tempQueue,
                                 state->callbacks,
@@ -1319,7 +1301,6 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
                                 state);
 
     /* Purge any deleted callbacks */
-    virDomainEventStateLock(state);
     virDomainEventCallbackListPurgeMarked(state->callbacks);
 
     state->isDispatching = false;
@@ -1346,10 +1327,32 @@ int virDomainEventStateRegister(virConnectPtr conn,
                                 void *opaque,
                                 virFreeCallback freecb)
 {
-    int ret;
+    int ret = -1;
+
     virDomainEventStateLock(state);
+
+    if ((state->callbacks->count == 0) &&
+        (state->timer == -1) &&
+        (state->timer = virEventAddTimeout(-1,
+                                           virDomainEventTimer,
+                                           state,
+                                           NULL)) < 0) {
+        eventReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                         _("could not initialize domain event timer"));
+        goto cleanup;
+    }
+
     ret = virDomainEventCallbackListAdd(conn, state->callbacks,
                                         callback, opaque, freecb);
+
+    if (ret == -1 &&
+        state->callbacks->count == 0 &&
+        state->timer != -1) {
+        virEventRemoveTimeout(state->timer);
+        state->timer = -1;
+    }
+
+cleanup:
     virDomainEventStateUnlock(state);
     return ret;
 }
@@ -1379,11 +1382,33 @@ int virDomainEventStateRegisterID(virConnectPtr conn,
                                   virFreeCallback freecb,
                                   int *callbackID)
 {
-    int ret;
+    int ret = -1;
+
     virDomainEventStateLock(state);
+
+    if ((state->callbacks->count == 0) &&
+        (state->timer == -1) &&
+        (state->timer = virEventAddTimeout(-1,
+                                           virDomainEventTimer,
+                                           state,
+                                           NULL)) < 0) {
+        eventReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                         _("could not initialize domain event timer"));
+        goto cleanup;
+    }
+
     ret = virDomainEventCallbackListAddID(conn, state->callbacks,
                                           dom, eventID, cb, opaque, freecb,
                                           callbackID);
+
+    if (ret == -1 &&
+        state->callbacks->count == 0 &&
+        state->timer != -1) {
+        virEventRemoveTimeout(state->timer);
+        state->timer = -1;
+    }
+
+cleanup:
     virDomainEventStateUnlock(state);
     return ret;
 }
@@ -1413,6 +1438,13 @@ virDomainEventStateDeregister(virConnectPtr conn,
                                                    state->callbacks, callback);
     else
         ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback);
+
+    if (state->callbacks->count == 0 &&
+        state->timer != -1) {
+        virEventRemoveTimeout(state->timer);
+        state->timer = -1;
+    }
+
     virDomainEventStateUnlock(state);
     return ret;
 }
@@ -1443,6 +1475,13 @@ virDomainEventStateDeregisterID(virConnectPtr conn,
     else
         ret = virDomainEventCallbackListRemoveID(conn,
                                                  state->callbacks, callbackID);
+
+    if (state->callbacks->count == 0 &&
+        state->timer != -1) {
+        virEventRemoveTimeout(state->timer);
+        state->timer = -1;
+    }
+
     virDomainEventStateUnlock(state);
     return ret;
 }
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
index a6c3562..0e7cd75 100644
--- a/src/conf/domain_event.h
+++ b/src/conf/domain_event.h
@@ -119,7 +119,7 @@ void virDomainEventFree(virDomainEventPtr event);
 
 void virDomainEventStateFree(virDomainEventStatePtr state);
 virDomainEventStatePtr
-virDomainEventStateNew(bool requireTimer);
+virDomainEventStateNew(void);
 
 void
 virDomainEventStateQueue(virDomainEventStatePtr state,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 04392da..0500ed0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -928,7 +928,7 @@ libxlStartup(int privileged) {
     }
     VIR_FREE(log_file);
 
-    libxl_driver->domainEventState = virDomainEventStateNew(true);
+    libxl_driver->domainEventState = virDomainEventStateNew();
     if (!libxl_driver->domainEventState)
         goto error;
 
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 6d32ed2..3baff19 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2418,7 +2418,7 @@ static int lxcStartup(int privileged)
     if (virDomainObjListInit(&lxc_driver->domains) < 0)
         goto cleanup;
 
-    lxc_driver->domainEventState = virDomainEventStateNew(true);
+    lxc_driver->domainEventState = virDomainEventStateNew();
     if (!lxc_driver->domainEventState)
         goto cleanup;
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b2a8525..5a876de 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -431,7 +431,7 @@ qemudStartup(int privileged) {
         goto out_of_memory;
 
     /* Init domain events */
-    qemu_driver->domainEventState = virDomainEventStateNew(true);
+    qemu_driver->domainEventState = virDomainEventStateNew();
     if (!qemu_driver->domainEventState)
         goto error;
 
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 03bd424..f266574 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -726,7 +726,7 @@ doRemoteOpen (virConnectPtr conn,
         }
     }
 
-    if (!(priv->domainEventState = virDomainEventStateNew(false)))
+    if (!(priv->domainEventState = virDomainEventStateNew()))
         goto failed;
 
     /* Successful. */
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5ec4190..9e00b1c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1137,7 +1137,7 @@ static virDrvOpenStatus testOpen(virConnectPtr conn,
     privconn = conn->privateData;
     testDriverLock(privconn);
 
-    privconn->domainEventState = virDomainEventStateNew(false);
+    privconn->domainEventState = virDomainEventStateNew();
     if (!privconn->domainEventState) {
         testDriverUnlock(privconn);
         testClose(conn);
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 360f0ce..671216e 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -413,7 +413,7 @@ umlStartup(int privileged)
     if (virDomainObjListInit(&uml_driver->domains) < 0)
         goto error;
 
-    uml_driver->domainEventState = virDomainEventStateNew(true);
+    uml_driver->domainEventState = virDomainEventStateNew();
     if (!uml_driver->domainEventState)
         goto error;
 
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index a10f5ae..0339123 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -1034,7 +1034,7 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn,
 
 #else  /* !(VBOX_API_VERSION == 2002) */
 
-    if (!(data->domainEvents = virDomainEventStateNew(true))) {
+    if (!(data->domainEvents = virDomainEventStateNew())) {
         vboxUninitialize(data);
         return VIR_DRV_OPEN_ERROR;
     }
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index ab49c2b..20671c0 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -325,7 +325,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags)
         return VIR_DRV_OPEN_ERROR;
     }
 
-    if (!(priv->domainEvents = virDomainEventStateNew(true))) {
+    if (!(priv->domainEvents = virDomainEventStateNew())) {
         virMutexDestroy(&priv->lock);
         VIR_FREE(priv);
         return VIR_DRV_OPEN_ERROR;
-- 
1.7.7.4


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