From: "Daniel P. Berrange" <berrange@redhat.com> The Xen & VBox drivers deal with callbacks & dispatching of events directly. All the other drivers use a timer to dispatch events from a clean stack state, rather than deep inside the drivers. Convert Xen & VBox over to virDomainEventState so that they match behaviour of other drivers * src/conf/domain_event.c: Return count of remaining callbacks when unregistering event callback * src/vbox/vbox_tmpl.c, src/xen/xen_driver.c, src/xen/xen_driver.h: Convert to virDomainEventState --- src/conf/domain_event.c | 58 ++++++++++++++++-- src/conf/domain_event.h | 6 +- src/vbox/vbox_tmpl.c | 146 +++++++++++++++++++++++++---------------------- src/xen/xen_driver.c | 68 ++++++++++------------ src/xen/xen_driver.h | 4 +- 5 files changed, 164 insertions(+), 118 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 614ab97..0e481cf 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -133,6 +133,7 @@ virDomainEventCallbackListRemove(virConnectPtr conn, virDomainEventCallbackListPtr cbList, virConnectDomainEventCallback callback) { + int ret = 0; int i; for (i = 0 ; i < cbList->count ; i++) { if (cbList->callbacks[i]->cb == VIR_DOMAIN_EVENT_CALLBACK(callback) && @@ -156,7 +157,11 @@ virDomainEventCallbackListRemove(virConnectPtr conn, } cbList->count--; - return 0; + for (i = 0 ; i < cbList->count ; i++) { + if (!cbList->callbacks[i]->deleted) + ret++; + } + return ret; } } @@ -179,6 +184,7 @@ virDomainEventCallbackListRemoveID(virConnectPtr conn, virDomainEventCallbackListPtr cbList, int callbackID) { + int ret = 0; int i; for (i = 0 ; i < cbList->count ; i++) { if (cbList->callbacks[i]->callbackID == callbackID && @@ -201,7 +207,11 @@ virDomainEventCallbackListRemoveID(virConnectPtr conn, } cbList->count--; - return 0; + for (i = 0 ; i < cbList->count ; i++) { + if (!cbList->callbacks[i]->deleted) + ret++; + } + return ret; } } @@ -254,13 +264,18 @@ int virDomainEventCallbackListMarkDelete(virConnectPtr conn, virDomainEventCallbackListPtr cbList, virConnectDomainEventCallback callback) { + int ret = 0; int i; for (i = 0 ; i < cbList->count ; i++) { if (cbList->callbacks[i]->cb == VIR_DOMAIN_EVENT_CALLBACK(callback) && cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE && cbList->callbacks[i]->conn == conn) { cbList->callbacks[i]->deleted = 1; - return 0; + for (i = 0 ; i < cbList->count ; i++) { + if (!cbList->callbacks[i]->deleted) + ret++; + } + return ret; } } @@ -274,12 +289,17 @@ int virDomainEventCallbackListMarkDeleteID(virConnectPtr conn, virDomainEventCallbackListPtr cbList, int callbackID) { + int ret = 0; int i; for (i = 0 ; i < cbList->count ; i++) { if (cbList->callbacks[i]->callbackID == callbackID && cbList->callbacks[i]->conn == conn) { cbList->callbacks[i]->deleted = 1; - return 0; + for (i = 0 ; i < cbList->count ; i++) { + if (!cbList->callbacks[i]->deleted) + ret++; + } + return ret; } } @@ -1307,6 +1327,18 @@ virDomainEventStateFlush(virDomainEventStatePtr state, virDomainEventStateUnlock(state); } + +/** + * virDomainEventStateDeregister: + * @state: domain event state + * @conn: connection to associate with callback + * @callback: function to remove from event + * + * Unregister the function @callback with connection @conn, + * from @state, for lifecycle events. + * + * Returns: the number of lifecycle callbacks still registered, or -1 on error + */ int virDomainEventStateDeregister(virConnectPtr conn, virDomainEventStatePtr state, @@ -1324,10 +1356,22 @@ virDomainEventStateDeregister(virConnectPtr conn, return ret; } + +/** + * virDomainEventStateDeregisterID: + * @state: domain event state + * @conn: connection to associate with callback + * @callbackID: ID of the function to remove from event + * + * Unregister the function @callbackID with connection @conn, + * from @state, for lifecycle events. + * + * Returns: the number of lifecycle callbacks still registered, or -1 on error + */ int -virDomainEventStateDeregisterAny(virConnectPtr conn, - virDomainEventStatePtr state, - int callbackID) +virDomainEventStateDeregisterID(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID) { int ret; diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 3ba418e..10a55b0 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -242,9 +242,9 @@ virDomainEventStateDeregister(virConnectPtr conn, virConnectDomainEventCallback callback) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int -virDomainEventStateDeregisterAny(virConnectPtr conn, - virDomainEventStatePtr state, - int callbackID) +virDomainEventStateDeregisterID(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 9b74a7b..7f13f90 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -188,11 +188,9 @@ typedef struct { #else /* !(VBOX_API_VERSION == 2002) */ - /* An array of callbacks */ - virDomainEventCallbackListPtr domainEventCallbacks; - + /* Async event handling */ + virDomainEventStatePtr domainEvents; int fdWatch; - int domainEventDispatching; # if VBOX_API_VERSION <= 3002 /* IVirtualBoxCallback is used in VirtualBox 3.x only */ @@ -966,11 +964,52 @@ static void vboxUninitialize(vboxGlobalData *data) { #if VBOX_API_VERSION == 2002 /* No domainEventCallbacks in 2.2.* version */ #else /* !(VBOX_API_VERSION == 2002) */ - VIR_FREE(data->domainEventCallbacks); + virDomainEventStateFree(data->domainEvents); #endif /* !(VBOX_API_VERSION == 2002) */ VIR_FREE(data); } + +#if VBOX_API_VERSION == 2002 + /* No domainEventCallbacks in 2.2.* version */ +#else /* !(VBOX_API_VERSION == 2002) */ + +static void +vboxDomainEventDispatchFunc(virConnectPtr conn, + virDomainEventPtr event, + virConnectDomainEventGenericCallback cb, + void *cbopaque, + void *opaque) +{ + vboxGlobalData *data = opaque; + + /* + * Release the lock while the callback is running so that + * we're re-entrant safe for callback work - the callback + * may want to invoke other virt functions & we have already + * protected the one piece of state we have - the callback + * list + */ + vboxDriverUnlock(data); + virDomainEventDispatchDefaultFunc(conn, event, cb, cbopaque, NULL); + vboxDriverLock(data); +} + + +static void vboxDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) +{ + virConnectPtr conn = opaque; + vboxGlobalData *data = conn->privateData; + + vboxDriverLock(data); + virDomainEventStateFlush(data->domainEvents, + vboxDomainEventDispatchFunc, + data); + vboxDriverUnlock(data); +} +#endif /* !(VBOX_API_VERSION == 2002) */ + + static virDrvOpenStatus vboxOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) @@ -1035,8 +1074,11 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn, #else /* !(VBOX_API_VERSION == 2002) */ - if (VIR_ALLOC(data->domainEventCallbacks) < 0) { - virReportOOMError(); + if (!(data->domainEvents = virDomainEventStateNew(vboxDomainEventFlush, + data, + NULL, + true))) { + vboxUninitialize(data); return VIR_DRV_OPEN_ERROR; } @@ -6770,7 +6812,6 @@ vboxCallbackOnMachineStateChange(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, int event = 0; int detail = 0; - g_pVBoxGlobalData->domainEventDispatching = 1; vboxDriverLock(g_pVBoxGlobalData); VIR_DEBUG("IVirtualBoxCallback: %p, State: %d", pThis, state); @@ -6818,20 +6859,12 @@ vboxCallbackOnMachineStateChange(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, ev = virDomainEventNewFromDom(dom, event, detail); - if (ev) { - virDomainEventDispatch(ev, - g_pVBoxGlobalData->domainEventCallbacks, - virDomainEventDispatchDefaultFunc, - NULL); - virDomainEventFree(ev); - } + if (ev) + virDomainEventStateQueue(g_pVBoxGlobalData->domainEvents, ev); } } - virDomainEventCallbackListPurgeMarked(g_pVBoxGlobalData->domainEventCallbacks); - vboxDriverUnlock(g_pVBoxGlobalData); - g_pVBoxGlobalData->domainEventDispatching = 0; return NS_OK; } @@ -6898,7 +6931,6 @@ vboxCallbackOnMachineRegistered(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, int event = 0; int detail = 0; - g_pVBoxGlobalData->domainEventDispatching = 1; vboxDriverLock(g_pVBoxGlobalData); VIR_DEBUG("IVirtualBoxCallback: %p, registered: %s", pThis, registered ? "true" : "false"); @@ -6931,20 +6963,12 @@ vboxCallbackOnMachineRegistered(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, ev = virDomainEventNewFromDom(dom, event, detail); - if (ev) { - virDomainEventDispatch(ev, - g_pVBoxGlobalData->domainEventCallbacks, - virDomainEventDispatchDefaultFunc, - NULL); - virDomainEventFree(ev); - } + if (ev) + virDomainEventStateQueue(g_pVBoxGlobalData->domainEvents, ev); } } - virDomainEventCallbackListPurgeMarked(g_pVBoxGlobalData->domainEventCallbacks); - vboxDriverUnlock(g_pVBoxGlobalData); - g_pVBoxGlobalData->domainEventDispatching = 0; return NS_OK; } @@ -7164,11 +7188,11 @@ static int vboxDomainEventRegister (virConnectPtr conn, * later you can iterate over them */ - ret = virDomainEventCallbackListAdd(conn, data->domainEventCallbacks, + ret = virDomainEventCallbackListAdd(conn, data->domainEvents->callbacks, callback, opaque, freecb); VIR_DEBUG("virDomainEventCallbackListAdd (ret = %d) ( conn: %p, " - "data->domainEventCallbacks: %p, callback: %p, opaque: %p, " - "freecb: %p )", ret, conn, data->domainEventCallbacks, callback, + "data->domainEvents->callbacks: %p, callback: %p, opaque: %p, " + "freecb: %p )", ret, conn, data->domainEvents->callbacks, callback, opaque, freecb); } } @@ -7188,31 +7212,23 @@ static int vboxDomainEventRegister (virConnectPtr conn, static int vboxDomainEventDeregister (virConnectPtr conn, virConnectDomainEventCallback callback) { VBOX_OBJECT_CHECK(conn, int, -1); + int cnt; /* Locking has to be there as callbacks are not * really fully thread safe */ vboxDriverLock(data); - if (data->domainEventDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, data->domainEventCallbacks, - callback); - else - ret = virDomainEventCallbackListRemove(conn, data->domainEventCallbacks, - callback); + cnt = virDomainEventStateDeregister(conn, data->domainEvents, + callback); - if (data->vboxCallback) { - /* check count here of how many times register was called - * and only on the last de-register do the un-register call - */ - if (data->domainEventCallbacks && virDomainEventCallbackListCount(data->domainEventCallbacks) == 0) { - data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); - VBOX_RELEASE(data->vboxCallback); + if (data->vboxCallback && cnt == 0) { + data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); + VBOX_RELEASE(data->vboxCallback); - /* Remove the Event file handle on which we are listening as well */ - virEventRemoveHandle(data->fdWatch); - data->fdWatch = -1; - } + /* Remove the Event file handle on which we are listening as well */ + virEventRemoveHandle(data->fdWatch); + data->fdWatch = -1; } vboxDriverUnlock(data); @@ -7264,12 +7280,12 @@ static int vboxDomainEventRegisterAny(virConnectPtr conn, * later you can iterate over them */ - ret = virDomainEventCallbackListAddID(conn, data->domainEventCallbacks, + ret = virDomainEventCallbackListAddID(conn, data->domainEvents->callbacks, dom, eventID, callback, opaque, freecb); VIR_DEBUG("virDomainEventCallbackListAddID (ret = %d) ( conn: %p, " - "data->domainEventCallbacks: %p, callback: %p, opaque: %p, " - "freecb: %p )", ret, conn, data->domainEventCallbacks, callback, + "data->domainEvents->callbacks: %p, callback: %p, opaque: %p, " + "freecb: %p )", ret, conn, data->domainEvents->callbacks, callback, opaque, freecb); } } @@ -7289,31 +7305,23 @@ static int vboxDomainEventRegisterAny(virConnectPtr conn, static int vboxDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { VBOX_OBJECT_CHECK(conn, int, -1); + int cnt; /* Locking has to be there as callbacks are not * really fully thread safe */ vboxDriverLock(data); - if (data->domainEventDispatching) - ret = virDomainEventCallbackListMarkDeleteID(conn, data->domainEventCallbacks, - callbackID); - else - ret = virDomainEventCallbackListRemoveID(conn, data->domainEventCallbacks, - callbackID); + cnt = virDomainEventStateDeregisterAny(conn, data->domainEvents, + callbackID); - if (data->vboxCallback) { - /* check count here of how many times register was called - * and only on the last de-register do the un-register call - */ - if (data->domainEventCallbacks && virDomainEventCallbackListCount(data->domainEventCallbacks) == 0) { - data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); - VBOX_RELEASE(data->vboxCallback); + if (data->vboxCallback && cnt == 0) { + data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); + VBOX_RELEASE(data->vboxCallback); - /* Remove the Event file handle on which we are listening as well */ - virEventRemoveHandle(data->fdWatch); - data->fdWatch = -1; - } + /* Remove the Event file handle on which we are listening as well */ + virEventRemoveHandle(data->fdWatch); + data->fdWatch = -1; } vboxDriverUnlock(data); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index d394ff7..8c47ad5 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -63,6 +63,8 @@ static int xenUnifiedDomainGetVcpus (virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, unsigned char *cpumaps, int maplen); +static void xenDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque); + /* The five Xen drivers below us. */ static struct xenUnifiedDriver const * const drivers[XEN_UNIFIED_NR_DRIVERS] = { @@ -248,12 +250,13 @@ xenUnifiedXendProbe (void) } #endif + + static virDrvOpenStatus xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) { int i, ret = VIR_DRV_OPEN_DECLINED; xenUnifiedPrivatePtr priv; - virDomainEventCallbackListPtr cbList; #ifdef __sun /* @@ -323,17 +326,16 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) return VIR_DRV_OPEN_ERROR; } - /* Allocate callback list */ - if (VIR_ALLOC(cbList) < 0) { - virReportOOMError(); + if (!(priv->domainEvents = virDomainEventStateNew(xenDomainEventFlush, + priv, + NULL, + NULL))) { virMutexDestroy(&priv->lock); VIR_FREE(priv); return VIR_DRV_OPEN_ERROR; } conn->privateData = priv; - priv->domainEventCallbacks = cbList; - priv->handle = -1; priv->xendConfigVersion = -1; priv->xshandle = NULL; @@ -423,7 +425,7 @@ xenUnifiedClose (virConnectPtr conn) int i; virCapabilitiesFree(priv->caps); - virDomainEventCallbackListFree(priv->domainEventCallbacks); + virDomainEventStateFree(priv->domainEvents); for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) if (priv->opened[i]) @@ -1845,7 +1847,7 @@ xenUnifiedDomainEventRegister(virConnectPtr conn, return -1; } - ret = virDomainEventCallbackListAdd(conn, priv->domainEventCallbacks, + ret = virDomainEventCallbackListAdd(conn, priv->domainEvents->callbacks, callback, opaque, freefunc); xenUnifiedUnlock(priv); @@ -1867,12 +1869,9 @@ xenUnifiedDomainEventDeregister(virConnectPtr conn, return -1; } - if (priv->domainEventDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, priv->domainEventCallbacks, - callback); - else - ret = virDomainEventCallbackListRemove(conn, priv->domainEventCallbacks, - callback); + ret = virDomainEventStateDeregister(conn, + priv->domainEvents, + callback); xenUnifiedUnlock(priv); return ret; @@ -1898,7 +1897,7 @@ xenUnifiedDomainEventRegisterAny(virConnectPtr conn, return -1; } - ret = virDomainEventCallbackListAddID(conn, priv->domainEventCallbacks, + ret = virDomainEventCallbackListAddID(conn, priv->domainEvents->callbacks, dom, eventID, callback, opaque, freefunc); @@ -1920,12 +1919,9 @@ xenUnifiedDomainEventDeregisterAny(virConnectPtr conn, return -1; } - if (priv->domainEventDispatching) - ret = virDomainEventCallbackListMarkDeleteID(conn, priv->domainEventCallbacks, - callbackID); - else - ret = virDomainEventCallbackListRemoveID(conn, priv->domainEventCallbacks, - callbackID); + ret = virDomainEventStateDeregisterAny(conn, + priv->domainEvents, + callbackID); xenUnifiedUnlock(priv); return ret; @@ -2390,6 +2386,7 @@ xenUnifiedRemoveDomainInfo(xenUnifiedDomainInfoListPtr list, return -1; } + static void xenUnifiedDomainEventDispatchFunc(virConnectPtr conn, virDomainEventPtr event, @@ -2411,6 +2408,19 @@ xenUnifiedDomainEventDispatchFunc(virConnectPtr conn, xenUnifiedLock(priv); } +static void xenDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) +{ + virConnectPtr conn = opaque; + xenUnifiedPrivatePtr priv = conn->privateData; + + xenUnifiedLock(priv); + virDomainEventStateFlush(priv->domainEvents, + xenUnifiedDomainEventDispatchFunc, + priv); + xenUnifiedUnlock(priv); +} + + /** * xenUnifiedDomainEventDispatch: * @priv: the connection to dispatch events on @@ -2427,21 +2437,7 @@ void xenUnifiedDomainEventDispatch (xenUnifiedPrivatePtr priv, if (!priv) return; - priv->domainEventDispatching = 1; - - if (priv->domainEventCallbacks) { - virDomainEventDispatch(event, - priv->domainEventCallbacks, - xenUnifiedDomainEventDispatchFunc, - priv); - - /* Purge any deleted callbacks */ - virDomainEventCallbackListPurgeMarked(priv->domainEventCallbacks); - } - - virDomainEventFree(event); - - priv->domainEventDispatching = 0; + virDomainEventStateQueue(priv->domainEvents, event); } void xenUnifiedLock(xenUnifiedPrivatePtr priv) diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index ca711d4..4122378 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -183,9 +183,7 @@ struct _xenUnifiedPrivate { int nbNodeCells; int nbNodeCpus; - /* An list of callbacks */ - virDomainEventCallbackListPtr domainEventCallbacks; - int domainEventDispatching; + virDomainEventStatePtr domainEvents; /* Location of config files, either /etc * or /var/lib/xen */ -- 1.7.7.4

From: "Daniel P. Berrange" <berrange@redhat.com> When registering a callback for a particular event some callers need to know how many callbacks already exist for that event. While it is possible to ask for a count, this is not free from race conditions when threaded. Thus the API for registering callbacks should return the count of callbacks * src/conf/domain_event.c, src/conf/domain_event.h, src/libvirt_private.syms: Return count of callbacks when registering callbacks * src/libxl/libxl_driver.c, src/libxl/libxl_driver.c, src/qemu/qemu_driver.c, src/remote/remote_driver.c, src/remote/remote_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c, src/xen/xen_driver.c: Update for change in APIs --- src/conf/domain_event.c | 23 ++++++++++++++++++----- src/conf/domain_event.h | 3 ++- src/libvirt_private.syms | 2 +- src/libxl/libxl_driver.c | 15 ++++++++------- src/lxc/lxc_driver.c | 15 ++++++++------- src/qemu/qemu_driver.c | 15 ++++++++------- src/remote/remote_driver.c | 33 ++++++++++++++++----------------- src/test/test_driver.c | 15 ++++++++------- src/uml/uml_driver.c | 15 ++++++++------- src/vbox/vbox_tmpl.c | 15 ++++++++------- src/xen/xen_driver.c | 13 +++++++------ 11 files changed, 92 insertions(+), 72 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 0e481cf..00c5dbf 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -357,7 +357,7 @@ virDomainEventCallbackListAdd(virConnectPtr conn, return virDomainEventCallbackListAddID(conn, cbList, NULL, VIR_DOMAIN_EVENT_ID_LIFECYCLE, VIR_DOMAIN_EVENT_CALLBACK(callback), - opaque, freecb); + opaque, freecb, NULL); } @@ -368,6 +368,7 @@ virDomainEventCallbackListAdd(virConnectPtr conn, * @eventID: the event ID * @callback: the callback to add * @opaque: opaque data tio pass to callback + * @callbackID: filled with callback ID * * Internal function to add a callback from a virDomainEventCallbackListPtr */ @@ -378,10 +379,12 @@ virDomainEventCallbackListAddID(virConnectPtr conn, int eventID, virConnectDomainEventGenericCallback callback, void *opaque, - virFreeCallback freecb) + virFreeCallback freecb, + int *callbackID) { virDomainEventCallbackPtr event; int i; + int ret = 0; /* Check incoming */ if ( !cbList ) { @@ -427,7 +430,17 @@ virDomainEventCallbackListAddID(virConnectPtr conn, event->callbackID = cbList->nextID++; - return event->callbackID; + for (i = 0 ; i < cbList->count ; i++) { + if (cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE && + cbList->callbacks[i]->conn == conn && + !cbList->callbacks[i]->deleted) + ret++; + } + + if (callbackID) + *callbackID = event->callbackID; + + return ret; no_memory: virReportOOMError(); @@ -1364,9 +1377,9 @@ virDomainEventStateDeregister(virConnectPtr conn, * @callbackID: ID of the function to remove from event * * Unregister the function @callbackID with connection @conn, - * from @state, for lifecycle events. + * from @state, for events. * - * Returns: the number of lifecycle callbacks still registered, or -1 on error + * Returns: the number of callbacks still registered, or -1 on error */ int virDomainEventStateDeregisterID(virConnectPtr conn, diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 10a55b0..83656e6 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -80,7 +80,8 @@ int virDomainEventCallbackListAddID(virConnectPtr conn, int eventID, virConnectDomainEventGenericCallback cb, void *opaque, - virFreeCallback freecb) + virFreeCallback freecb, + int *callbackID) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(5); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a81c230..3d1d7d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -510,7 +510,7 @@ virDomainEventRebootNew; virDomainEventRebootNewFromDom; virDomainEventRebootNewFromObj; virDomainEventStateDeregister; -virDomainEventStateDeregisterAny; +virDomainEventStateDeregisterID; virDomainEventStateFlush; virDomainEventStateFree; virDomainEventStateNew; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7cc32ad..46504dc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3851,10 +3851,11 @@ libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, int ret; libxlDriverLock(driver); - ret = virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, callback, opaque, - freecb); + if (virDomainEventCallbackListAddID(conn, + driver->domainEventState->callbacks, + dom, eventID, callback, opaque, + freecb, &ret) < 0) + ret = -1; libxlDriverUnlock(driver); return ret; @@ -3868,9 +3869,9 @@ libxlDomainEventDeregisterAny(virConnectPtr conn, int callbackID) int ret; libxlDriverLock(driver); - ret = virDomainEventStateDeregisterAny(conn, - driver->domainEventState, - callbackID); + ret = virDomainEventStateDeregisterID(conn, + driver->domainEventState, + callbackID); libxlDriverUnlock(driver); return ret; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b16cfd8..6b17c75 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2164,10 +2164,11 @@ lxcDomainEventRegisterAny(virConnectPtr conn, int ret; lxcDriverLock(driver); - ret = virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb); + if (virDomainEventCallbackListAddID(conn, + driver->domainEventState->callbacks, + dom, eventID, + callback, opaque, freecb, &ret) < 0) + ret = -1; lxcDriverUnlock(driver); return ret; @@ -2182,9 +2183,9 @@ lxcDomainEventDeregisterAny(virConnectPtr conn, int ret; lxcDriverLock(driver); - ret = virDomainEventStateDeregisterAny(conn, - driver->domainEventState, - callbackID); + ret = virDomainEventStateDeregisterID(conn, + driver->domainEventState, + callbackID); lxcDriverUnlock(driver); return ret; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 10a289e..7423340 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8180,10 +8180,11 @@ qemuDomainEventRegisterAny(virConnectPtr conn, int ret; qemuDriverLock(driver); - ret = virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb); + if (virDomainEventCallbackListAddID(conn, + driver->domainEventState->callbacks, + dom, eventID, + callback, opaque, freecb, &ret) < 0) + ret = -1; qemuDriverUnlock(driver); return ret; @@ -8198,9 +8199,9 @@ qemuDomainEventDeregisterAny(virConnectPtr conn, int ret; qemuDriverLock(driver); - ret = virDomainEventStateDeregisterAny(conn, - driver->domainEventState, - callbackID); + ret = virDomainEventStateDeregisterID(conn, + driver->domainEventState, + callbackID); qemuDriverUnlock(driver); return ret; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ff2d4b4..a0cf7d3 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3120,6 +3120,7 @@ static int remoteDomainEventRegister(virConnectPtr conn, { int rv = -1; struct private_data *priv = conn->privateData; + int count; remoteDriverLock(priv); @@ -3128,15 +3129,13 @@ static int remoteDomainEventRegister(virConnectPtr conn, goto done; } - if (virDomainEventCallbackListAdd(conn, priv->domainEventState->callbacks, - callback, opaque, freecb) < 0) { + if ((count = virDomainEventCallbackListAdd(conn, priv->domainEventState->callbacks, + callback, opaque, freecb)) < 0) { remoteError(VIR_ERR_RPC, "%s", _("adding cb to list")); goto done; } - if (virDomainEventCallbackListCountID(conn, - priv->domainEventState->callbacks, - VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 1) { + if (count == 1) { /* Tell the server when we are the first callback deregistering */ if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_REGISTER, (xdrproc_t) xdr_void, (char *) NULL, @@ -3760,6 +3759,7 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn, struct private_data *priv = conn->privateData; remote_domain_events_register_any_args args; int callbackID; + int count; remoteDriverLock(priv); @@ -3768,19 +3768,18 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn, goto done; } - if ((callbackID = virDomainEventCallbackListAddID(conn, - priv->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb)) < 0) { - remoteError(VIR_ERR_RPC, "%s", _("adding cb to list")); - goto done; + if ((count = virDomainEventCallbackListAddID(conn, + priv->domainEventState->callbacks, + dom, eventID, + callback, opaque, freecb, + &callbackID)) < 0) { + remoteError(VIR_ERR_RPC, "%s", _("adding cb to list")); + goto done; } /* If this is the first callback for this eventID, we need to enable * events on the server */ - if (virDomainEventCallbackListCountID(conn, - priv->domainEventState->callbacks, - eventID) == 1) { + if (count == 1) { args.eventID = eventID; if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_REGISTER_ANY, @@ -3818,9 +3817,9 @@ static int remoteDomainEventDeregisterAny(virConnectPtr conn, goto done; } - if (virDomainEventStateDeregisterAny(conn, - priv->domainEventState, - callbackID) < 0) + if (virDomainEventStateDeregisterID(conn, + priv->domainEventState, + callbackID) < 0) goto done; /* If that was the last callback for this eventID, we need to disable diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 89f7df1..f32990b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5469,10 +5469,11 @@ testDomainEventRegisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - ret = virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb); + if (virDomainEventCallbackListAddID(conn, + driver->domainEventState->callbacks, + dom, eventID, + callback, opaque, freecb, &ret) < 0) + ret = -1; testDriverUnlock(driver); return ret; @@ -5486,9 +5487,9 @@ testDomainEventDeregisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - ret = virDomainEventStateDeregisterAny(conn, - driver->domainEventState, - callbackID); + ret = virDomainEventStateDeregisterID(conn, + driver->domainEventState, + callbackID); testDriverUnlock(driver); return ret; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index faea313..420488d 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2483,10 +2483,11 @@ umlDomainEventRegisterAny(virConnectPtr conn, int ret; umlDriverLock(driver); - ret = virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb); + if (virDomainEventCallbackListAddID(conn, + driver->domainEventState->callbacks, + dom, eventID, + callback, opaque, freecb, &ret) < 0) + ret = -1; umlDriverUnlock(driver); return ret; @@ -2501,9 +2502,9 @@ umlDomainEventDeregisterAny(virConnectPtr conn, int ret; umlDriverLock(driver); - ret = virDomainEventStateDeregisterAny(conn, - driver->domainEventState, - callbackID); + ret = virDomainEventStateDeregisterID(conn, + driver->domainEventState, + callbackID); umlDriverUnlock(driver); return ret; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 7f13f90..670de9c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -7280,13 +7280,14 @@ static int vboxDomainEventRegisterAny(virConnectPtr conn, * later you can iterate over them */ - ret = virDomainEventCallbackListAddID(conn, data->domainEvents->callbacks, - dom, eventID, - callback, opaque, freecb); + if (virDomainEventCallbackListAddID(conn, data->domainEvents->callbacks, + dom, eventID, + callback, opaque, freecb, &ret) < 0) + ret = -1; VIR_DEBUG("virDomainEventCallbackListAddID (ret = %d) ( conn: %p, " - "data->domainEvents->callbacks: %p, callback: %p, opaque: %p, " - "freecb: %p )", ret, conn, data->domainEvents->callbacks, callback, - opaque, freecb); + "data->domainEvents->callbacks: %p, callback: %p, opaque: %p, " + "freecb: %p )", ret, conn, data->domainEvents->callbacks, callback, + opaque, freecb); } } @@ -7312,7 +7313,7 @@ static int vboxDomainEventDeregisterAny(virConnectPtr conn, */ vboxDriverLock(data); - cnt = virDomainEventStateDeregisterAny(conn, data->domainEvents, + cnt = virDomainEventStateDeregisterID(conn, data->domainEvents, callbackID); if (data->vboxCallback && cnt == 0) { diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 8c47ad5..b11dd9e 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1897,9 +1897,10 @@ xenUnifiedDomainEventRegisterAny(virConnectPtr conn, return -1; } - ret = virDomainEventCallbackListAddID(conn, priv->domainEvents->callbacks, - dom, eventID, - callback, opaque, freefunc); + if (virDomainEventCallbackListAddID(conn, priv->domainEvents->callbacks, + dom, eventID, + callback, opaque, freefunc, &ret) < 0) + ret = -1; xenUnifiedUnlock(priv); return (ret); @@ -1919,9 +1920,9 @@ xenUnifiedDomainEventDeregisterAny(virConnectPtr conn, return -1; } - ret = virDomainEventStateDeregisterAny(conn, - priv->domainEvents, - callbackID); + ret = virDomainEventStateDeregisterID(conn, + priv->domainEvents, + callbackID); xenUnifiedUnlock(priv); return ret; -- 1.7.7.4

From: "Daniel P. Berrange" <berrange@redhat.com> While virDomainEventState has APIs for managing removal of callbacks, while locked, adding callbacks in the first place requires direct access to the virDomainEventCallbackList structure. This is not threadsafe since it is bypassing the virDomainEventState locks * src/conf/domain_event.c, src/conf/domain_event.h, src/libvirt_private.syms: Add APIs for managing callbacks via virDomainEventState. --- src/conf/domain_event.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_event.h | 24 +++++++++++ src/libvirt_private.syms | 4 ++ 3 files changed, 127 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 00c5dbf..c6adae4 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -1342,6 +1342,68 @@ virDomainEventStateFlush(virDomainEventStatePtr state, /** + * virDomainEventStateRegister: + * @state: domain event state + * @conn: connection to associate with callback + * @callback: function to remove from event + * @opaque: data blob to pass to callback + * @freecb: callback to free @opaque + * + * Register the function @callback with connection @conn, + * from @state, for lifecycle events. + * + * Returns: the number of lifecycle callbacks now registered, or -1 on error + */ +int virDomainEventStateRegister(virConnectPtr conn, + virDomainEventStatePtr state, + virConnectDomainEventCallback callback, + void *opaque, + virFreeCallback freecb) +{ + int ret; + virDomainEventStateLock(state); + ret = virDomainEventCallbackListAdd(conn, state->callbacks, + callback, opaque, freecb); + virDomainEventStateUnlock(state); + return ret; +} + + +/** + * virDomainEventStateRegisterID: + * @state: domain event state + * @conn: connection to associate with callback + * @eventID: ID of the event type to register for + * @cb: function to remove from event + * @opaque: data blob to pass to callback + * @freecb: callback to free @opaque + * @callbackID: filled with callback ID + * + * Register the function @callbackID with connection @conn, + * from @state, for events of type @eventID. + * + * Returns: the number of callbacks now registered, or -1 on error + */ +int virDomainEventStateRegisterID(virConnectPtr conn, + virDomainEventStatePtr state, + virDomainPtr dom, + int eventID, + virConnectDomainEventGenericCallback cb, + void *opaque, + virFreeCallback freecb, + int *callbackID) +{ + int ret; + virDomainEventStateLock(state); + ret = virDomainEventCallbackListAddID(conn, state->callbacks, + dom, eventID, cb, opaque, freecb, + callbackID); + virDomainEventStateUnlock(state); + return ret; +} + + +/** * virDomainEventStateDeregister: * @state: domain event state * @conn: connection to associate with callback @@ -1398,3 +1460,40 @@ virDomainEventStateDeregisterID(virConnectPtr conn, virDomainEventStateUnlock(state); return ret; } + + +/** + * virDomainEventStateDeregisterConn: + * @state: domain event state + * @conn: connection to associate with callbacks + * + * Remove all callbacks from @state associated with the + * connection @conn + * + * Returns 0 on success, -1 on error + */ +int +virDomainEventStateDeregisterConn(virConnectPtr conn, + virDomainEventStatePtr state) +{ + int ret; + virDomainEventStateLock(state); + ret = virDomainEventCallbackListRemoveConn(conn, state->callbacks); + virDomainEventStateUnlock(state); + return ret; +} + + +int +virDomainEventStateEventID(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID) +{ + int ret; + + virDomainEventStateLock(state); + ret = virDomainEventCallbackListEventID(conn, + state->callbacks, callbackID); + virDomainEventStateUnlock(state); + return ret; +} diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 83656e6..7eefadb 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -237,6 +237,21 @@ virDomainEventStateFlush(virDomainEventStatePtr state, virDomainEventDispatchFunc dispatchFunc, void *opaque) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virDomainEventStateRegister(virConnectPtr conn, + virDomainEventStatePtr state, + virConnectDomainEventCallback callback, + void *opaque, + virFreeCallback freecb) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int virDomainEventStateRegisterID(virConnectPtr conn, + virDomainEventStatePtr state, + virDomainPtr dom, + int eventID, + virConnectDomainEventGenericCallback cb, + void *opaque, + virFreeCallback freecb, + int *callbackID) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5); int virDomainEventStateDeregister(virConnectPtr conn, virDomainEventStatePtr state, @@ -247,5 +262,14 @@ virDomainEventStateDeregisterID(virConnectPtr conn, virDomainEventStatePtr state, int callbackID) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainEventStateDeregisterConn(virConnectPtr conn, + virDomainEventStatePtr state) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +virDomainEventStateEventID(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); #endif diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3d1d7d2..0c664ed 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -511,6 +511,10 @@ virDomainEventRebootNewFromDom; virDomainEventRebootNewFromObj; virDomainEventStateDeregister; virDomainEventStateDeregisterID; +virDomainEventStateDeregisterConn; +virDomainEventStateEventID; +virDomainEventStateRegister; +virDomainEventStateRegisterID; virDomainEventStateFlush; virDomainEventStateFree; virDomainEventStateNew; -- 1.7.7.4

From: "Daniel P. Berrange" <berrange@redhat.com> * src/libxl/libxl_driver.c, src/lxc/lxc_driver.c, src/qemu/qemu_driver.c, src/remote/remote_driver.c, src/test/test_driver.c, src/uml/uml_driver.c, src/vbox/vbox_tmpl.c, src/xen/xen_driver.c: Convert to threadsafe APIs --- src/libxl/libxl_driver.c | 18 ++++++------ src/lxc/lxc_driver.c | 18 ++++++------ src/qemu/qemu_driver.c | 18 ++++++------ src/remote/remote_driver.c | 64 ++++++++++++++++++------------------------- src/test/test_driver.c | 14 +++++----- src/uml/uml_driver.c | 18 ++++++------ src/vbox/vbox_tmpl.c | 24 ++++++++-------- src/xen/xen_driver.c | 10 +++--- 8 files changed, 87 insertions(+), 97 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 46504dc..b9382ee 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1115,8 +1115,8 @@ libxlClose(virConnectPtr conn ATTRIBUTE_UNUSED) libxlDriverPrivatePtr driver = conn->privateData; libxlDriverLock(driver); - virDomainEventCallbackListRemoveConn(conn, - driver->domainEventState->callbacks); + virDomainEventStateDeregisterConn(conn, + driver->domainEventState); libxlDriverUnlock(driver); conn->privateData = NULL; return 0; @@ -3404,9 +3404,9 @@ libxlDomainEventRegister(virConnectPtr conn, int ret; libxlDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, - driver->domainEventState->callbacks, - callback, opaque, freecb); + ret = virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb); libxlDriverUnlock(driver); return ret; @@ -3851,10 +3851,10 @@ libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, int ret; libxlDriverLock(driver); - if (virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, callback, opaque, - freecb, &ret) < 0) + if (virDomainEventStateRegisterID(conn, + driver->domainEventState, + dom, eventID, callback, opaque, + freecb, &ret) < 0) ret = -1; libxlDriverUnlock(driver); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 6b17c75..6a9ebde 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -179,8 +179,8 @@ static int lxcClose(virConnectPtr conn) lxc_driver_t *driver = conn->privateData; lxcDriverLock(driver); - virDomainEventCallbackListRemoveConn(conn, - driver->domainEventState->callbacks); + virDomainEventStateDeregisterConn(conn, + driver->domainEventState); lxcProcessAutoDestroyRun(driver, conn); lxcDriverUnlock(driver); @@ -2126,9 +2126,9 @@ lxcDomainEventRegister(virConnectPtr conn, int ret; lxcDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, - driver->domainEventState->callbacks, - callback, opaque, freecb); + ret = virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb); lxcDriverUnlock(driver); return ret; @@ -2164,10 +2164,10 @@ lxcDomainEventRegisterAny(virConnectPtr conn, int ret; lxcDriverLock(driver); - if (virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb, &ret) < 0) + if (virDomainEventStateRegisterID(conn, + driver->domainEventState, + dom, eventID, + callback, opaque, freecb, &ret) < 0) ret = -1; lxcDriverUnlock(driver); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7423340..40bd30f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -900,8 +900,8 @@ static int qemudClose(virConnectPtr conn) { /* Get rid of callbacks registered for this conn */ qemuDriverLock(driver); - virDomainEventCallbackListRemoveConn(conn, - driver->domainEventState->callbacks); + virDomainEventStateDeregisterConn(conn, + driver->domainEventState); qemuProcessAutoDestroyRun(driver, conn); qemuDriverUnlock(driver); @@ -8142,9 +8142,9 @@ qemuDomainEventRegister(virConnectPtr conn, int ret; qemuDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, - driver->domainEventState->callbacks, - callback, opaque, freecb); + ret = virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb); qemuDriverUnlock(driver); return ret; @@ -8180,10 +8180,10 @@ qemuDomainEventRegisterAny(virConnectPtr conn, int ret; qemuDriverLock(driver); - if (virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb, &ret) < 0) + if (virDomainEventStateRegisterID(conn, + driver->domainEventState, + dom, eventID, + callback, opaque, freecb, &ret) < 0) ret = -1; qemuDriverUnlock(driver); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index a0cf7d3..a10bcad 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3124,13 +3124,8 @@ static int remoteDomainEventRegister(virConnectPtr conn, remoteDriverLock(priv); - if (priv->domainEventState->timer < 0) { - remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support")); - goto done; - } - - if ((count = virDomainEventCallbackListAdd(conn, priv->domainEventState->callbacks, - callback, opaque, freecb)) < 0) { + if ((count = virDomainEventStateRegister(conn, priv->domainEventState, + callback, opaque, freecb)) < 0) { remoteError(VIR_ERR_RPC, "%s", _("adding cb to list")); goto done; } @@ -3155,17 +3150,16 @@ static int remoteDomainEventDeregister(virConnectPtr conn, { struct private_data *priv = conn->privateData; int rv = -1; + int count; remoteDriverLock(priv); - if (virDomainEventStateDeregister(conn, - priv->domainEventState, - callback) < 0) + if ((count = virDomainEventStateDeregister(conn, + priv->domainEventState, + callback)) < 0) goto done; - if (virDomainEventCallbackListCountID(conn, - priv->domainEventState->callbacks, - VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 0) { + if (count == 0) { /* Tell the server when we are the last callback deregistering */ if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER, (xdrproc_t) xdr_void, (char *) NULL, @@ -3763,20 +3757,15 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn, remoteDriverLock(priv); - if (priv->domainEventState->timer < 0) { - remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support")); + if ((count = virDomainEventStateRegisterID(conn, + priv->domainEventState, + dom, eventID, + callback, opaque, freecb, + &callbackID)) < 0) { + remoteError(VIR_ERR_RPC, "%s", _("adding cb to list")); goto done; } - if ((count = virDomainEventCallbackListAddID(conn, - priv->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb, - &callbackID)) < 0) { - remoteError(VIR_ERR_RPC, "%s", _("adding cb to list")); - goto done; - } - /* If this is the first callback for this eventID, we need to enable * events on the server */ if (count == 1) { @@ -3785,9 +3774,9 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn, if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_REGISTER_ANY, (xdrproc_t) xdr_remote_domain_events_register_any_args, (char *) &args, (xdrproc_t) xdr_void, (char *)NULL) == -1) { - virDomainEventCallbackListRemoveID(conn, - priv->domainEventState->callbacks, - callbackID); + virDomainEventStateDeregisterID(conn, + priv->domainEventState, + callbackID); goto done; } } @@ -3807,27 +3796,28 @@ static int remoteDomainEventDeregisterAny(virConnectPtr conn, int rv = -1; remote_domain_events_deregister_any_args args; int eventID; + int count; remoteDriverLock(priv); - if ((eventID = virDomainEventCallbackListEventID(conn, - priv->domainEventState->callbacks, - callbackID)) < 0) { + if ((eventID = virDomainEventStateEventID(conn, + priv->domainEventState, + callbackID)) < 0) { remoteError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID); goto done; } - if (virDomainEventStateDeregisterID(conn, - priv->domainEventState, - callbackID) < 0) + if ((count = virDomainEventStateDeregisterID(conn, + priv->domainEventState, + callbackID)) < 0) { + remoteError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID); goto done; + } /* If that was the last callback for this eventID, we need to disable * events on the server */ - if (virDomainEventCallbackListCountID(conn, - priv->domainEventState->callbacks, - eventID) == 0) { - args.eventID = eventID; + if (count == 0) { + args.eventID = callbackID; if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER_ANY, (xdrproc_t) xdr_remote_domain_events_deregister_any_args, (char *) &args, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f32990b..054a41a 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5431,9 +5431,9 @@ testDomainEventRegister(virConnectPtr conn, int ret; testDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, - driver->domainEventState->callbacks, - callback, opaque, freecb); + ret = virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb); testDriverUnlock(driver); return ret; @@ -5469,10 +5469,10 @@ testDomainEventRegisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - if (virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb, &ret) < 0) + if (virDomainEventStateRegisterID(conn, + driver->domainEventState, + dom, eventID, + callback, opaque, freecb, &ret) < 0) ret = -1; testDriverUnlock(driver); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 420488d..4d875c8 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1194,8 +1194,8 @@ static int umlClose(virConnectPtr conn) { struct uml_driver *driver = conn->privateData; umlDriverLock(driver); - virDomainEventCallbackListRemoveConn(conn, - driver->domainEventState->callbacks); + virDomainEventStateDeregisterConn(conn, + driver->domainEventState); umlProcessAutoDestroyRun(driver, conn); umlDriverUnlock(driver); @@ -2447,9 +2447,9 @@ umlDomainEventRegister(virConnectPtr conn, int ret; umlDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, - driver->domainEventState->callbacks, - callback, opaque, freecb); + ret = virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb); umlDriverUnlock(driver); return ret; @@ -2483,10 +2483,10 @@ umlDomainEventRegisterAny(virConnectPtr conn, int ret; umlDriverLock(driver); - if (virDomainEventCallbackListAddID(conn, - driver->domainEventState->callbacks, - dom, eventID, - callback, opaque, freecb, &ret) < 0) + if (virDomainEventStateRegisterID(conn, + driver->domainEventState, + dom, eventID, + callback, opaque, freecb, &ret) < 0) ret = -1; umlDriverUnlock(driver); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 670de9c..1fb369b 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -7188,12 +7188,12 @@ static int vboxDomainEventRegister (virConnectPtr conn, * later you can iterate over them */ - ret = virDomainEventCallbackListAdd(conn, data->domainEvents->callbacks, - callback, opaque, freecb); - VIR_DEBUG("virDomainEventCallbackListAdd (ret = %d) ( conn: %p, " - "data->domainEvents->callbacks: %p, callback: %p, opaque: %p, " - "freecb: %p )", ret, conn, data->domainEvents->callbacks, callback, - opaque, freecb); + ret = virDomainEventStateRegister(conn, data->domainEvents, + callback, opaque, freecb); + VIR_DEBUG("virDomainEventStateRegister (ret = %d) ( conn: %p, " + "callback: %p, opaque: %p, " + "freecb: %p )", ret, conn, callback, + opaque, freecb); } } @@ -7280,13 +7280,13 @@ static int vboxDomainEventRegisterAny(virConnectPtr conn, * later you can iterate over them */ - if (virDomainEventCallbackListAddID(conn, data->domainEvents->callbacks, - dom, eventID, - callback, opaque, freecb, &ret) < 0) + if (virDomainEventStateRegisterID(conn, data->domainEvents, + dom, eventID, + callback, opaque, freecb, &ret) < 0) ret = -1; - VIR_DEBUG("virDomainEventCallbackListAddID (ret = %d) ( conn: %p, " - "data->domainEvents->callbacks: %p, callback: %p, opaque: %p, " - "freecb: %p )", ret, conn, data->domainEvents->callbacks, callback, + VIR_DEBUG("virDomainEventStateRegisterID (ret = %d) ( conn: %p, " + "callback: %p, opaque: %p, " + "freecb: %p )", ret, conn, callback, opaque, freecb); } } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index b11dd9e..a2399dd 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1847,8 +1847,8 @@ xenUnifiedDomainEventRegister(virConnectPtr conn, return -1; } - ret = virDomainEventCallbackListAdd(conn, priv->domainEvents->callbacks, - callback, opaque, freefunc); + ret = virDomainEventStateRegister(conn, priv->domainEvents, + callback, opaque, freefunc); xenUnifiedUnlock(priv); return (ret); @@ -1897,9 +1897,9 @@ xenUnifiedDomainEventRegisterAny(virConnectPtr conn, return -1; } - if (virDomainEventCallbackListAddID(conn, priv->domainEvents->callbacks, - dom, eventID, - callback, opaque, freefunc, &ret) < 0) + if (virDomainEventStateRegisterID(conn, priv->domainEvents, + dom, eventID, + callback, opaque, freefunc, &ret) < 0) ret = -1; xenUnifiedUnlock(priv); -- 1.7.7.4

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(¨_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