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

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;
 }

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,

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;
 

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;
 

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;
 

src/remote/remote_driver.c

@@ -726,7 +726,7 @@ doRemoteOpen (virConnectPtr conn,
         }
     }
 
-    if (!(priv->domainEventState = virDomainEventStateNew(false)))
+    if (!(priv->domainEventState = virDomainEventStateNew()))
         goto failed;
 
     /* Successful. */

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);

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;
 

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;
     }

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;