Only add the timer when a callback is registered

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

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 44fdf787d983add12cd2b5d623b0b492c74726c7..3fd3ed2093558c1f8903ba01a22239e95e565f40 100644 (file)
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -632,13 +632,9 @@ 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;
 
@@ -659,23 +655,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;
 
@@ -1314,7 +1297,6 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
     state->queue->count = 0;
     state->queue->events = NULL;
     virEventUpdateTimeout(state->timer, -1);
-    virDomainEventStateUnlock(state);
 
     virDomainEventQueueDispatch(&tempQueue,
                                 state->callbacks,
@@ -1322,7 +1304,6 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
                                 state);
 
     /* Purge any deleted callbacks */
-    virDomainEventStateLock(state);
     virDomainEventCallbackListPurgeMarked(state->callbacks);
 
     state->isDispatching = false;
@@ -1350,10 +1331,32 @@ 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;
 }
@@ -1384,11 +1387,33 @@ 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;
 }
@@ -1418,6 +1443,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;
 }
@@ -1448,6 +1480,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 a6c3562ba99220a40772602c151151a7f4eae255..0e7cd75c31c522aa997774dae81776fdb5f41395 100644 (file)
--- 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 04392da981a5d6b0905c0530b52e612c87c5afd4..0500ed019e534ba877ac6ae97ed56edb71d4d6d4 100644 (file)
--- 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 6d32ed2ed4afe2c705c24608af1e9a98a825772b..3baff1935847757e6cdf8792eaafb5baa20d3b7e 100644 (file)
--- 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 36c61d7f241c87e0b5ffb1ee5a139e3775bcfb0d..660d771d19e39fdbde1e907d0c415a9310f3e812 100644 (file)
--- 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 3c0510b344d19bd99173294229b895ba1fa1b52d..b74b9d02ddd7801ccc0da4c24d089582d4358324 100644 (file)
--- 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 33750b5a15086dd50c81687033176020130c95d8..44aef3f8c39c26c5dbef8773224795f482ae1ac3 100644 (file)
--- 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 360f0ce4b9f1b8b4d716b77e5550b1e17d440591..671216e15ce8601034c77b8929dff3eb5bcc6f75 100644 (file)
--- 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 3b02d5ab0db718c9738d81fa313eaee79efb0e2e..d2ef7407ab7bba9e3c2a44c327af338575dc3310 100644 (file)
--- 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 ab49c2b34ac7c0190ea65c63bee8be44e6767732..20671c0d6af0804e5493491a6e118af05b773a63 100644 (file)
--- 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;