From 707781fe1269dc49fad98de0ca943047ccf1053a Mon Sep 17 00:00:00 2001
From: "Daniel P. Berrange" <berrange@redhat.com>
Date: Wed, 14 Dec 2011 00:25:58 +0000
Subject: [PATCH] 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 | 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 44fdf787d9..3fd3ed2093 100644
--- 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 a6c3562ba9..0e7cd75c31 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 04392da981..0500ed019e 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 6d32ed2ed4..3baff19358 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 36c61d7f24..660d771d19 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 3c0510b344..b74b9d02dd 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 33750b5a15..44aef3f8c3 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 360f0ce4b9..671216e15c 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -413,7 +413,7 @@ umlStartup(int privileged)
if (virDomainObjListInit(¨_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 3b02d5ab0d..d2ef7407ab 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 ab49c2b34a..20671c0d6a 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;