[libvirt] [PATCH] events: Propose a separate lock for event queue
Currently, push & pop from event queue (both server & client side) rely on lock from higher levels, e.g. on driver lock (qemu), private_data (remote), ...; This alone is not sufficient as not every function that interacts with this queue can/does lock, esp. in client where we have a different approach, "passing the buck". Therefore we need a separate lock just to protect event queue. For more info see: https://bugzilla.redhat.com/show_bug.cgi?id=743817 --- src/conf/domain_event.c | 65 +++++++++++++++++++++++++++++++++++++++++------ src/conf/domain_event.h | 1 + 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 3189346..6cc8168 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -547,6 +547,24 @@ virDomainEventQueuePtr virDomainEventQueueNew(void) return ret; } +static void +virDomainEventStateLock(virDomainEventStatePtr state) +{ + if (!state) + return; + + virMutexLock(&state->lock); +} + +static void +virDomainEventStateUnlock(virDomainEventStatePtr state) +{ + if (!state) + return; + + virMutexUnlock(&state->lock); +} + /** * virDomainEventStateFree: * @list: virDomainEventStatePtr to free @@ -564,6 +582,8 @@ virDomainEventStateFree(virDomainEventStatePtr state) if (state->timer != -1) virEventRemoveTimeout(state->timer); + + virMutexDestroy(&state->lock); VIR_FREE(state); } @@ -590,6 +610,13 @@ virDomainEventStateNew(virEventTimeoutCallback timeout_cb, goto error; } + if (virMutexInit(&state->lock) < 0) { + virReportSystemError(errno, "%s", + _("unable to initialize state mutex")); + VIR_FREE(state); + goto error; + } + if (VIR_ALLOC(state->callbacks) < 0) { virReportOOMError(); goto error; @@ -1161,18 +1188,26 @@ void virDomainEventStateQueue(virDomainEventStatePtr state, virDomainEventPtr event) { + int count; + if (state->timer < 0) { virDomainEventFree(event); return; } + virDomainEventStateLock(state); + if (virDomainEventQueuePush(state->queue, event) < 0) { VIR_DEBUG("Error adding event to queue"); virDomainEventFree(event); } - if (state->queue->count == 1) + count = state->queue->count; + virDomainEventStateUnlock(state); + + if (count == 1) virEventUpdateTimeout(state->timer, 0); + } void @@ -1182,6 +1217,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state, { virDomainEventQueue tempQueue; + virDomainEventStateLock(state); state->isDispatching = true; /* Copy the queue, so we're reentrant safe when dispatchFunc drops the @@ -1190,6 +1226,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state, tempQueue.events = state->queue->events; state->queue->count = 0; state->queue->events = NULL; + virDomainEventStateUnlock(state); virEventUpdateTimeout(state->timer, -1); virDomainEventQueueDispatch(&tempQueue, @@ -1198,9 +1235,11 @@ virDomainEventStateFlush(virDomainEventStatePtr state, opaque); /* Purge any deleted callbacks */ + virDomainEventStateLock(state); virDomainEventCallbackListPurgeMarked(state->callbacks); state->isDispatching = false; + virDomainEventStateUnlock(state); } int @@ -1208,11 +1247,16 @@ virDomainEventStateDeregister(virConnectPtr conn, virDomainEventStatePtr state, virConnectDomainEventCallback callback) { + int ret; + + virDomainEventStateLock(state); if (state->isDispatching) - return virDomainEventCallbackListMarkDelete(conn, - state->callbacks, callback); + ret = virDomainEventCallbackListMarkDelete(conn, + state->callbacks, callback); else - return virDomainEventCallbackListRemove(conn, state->callbacks, callback); + ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback); + virDomainEventStateUnlock(state); + return ret; } int @@ -1220,10 +1264,15 @@ virDomainEventStateDeregisterAny(virConnectPtr conn, virDomainEventStatePtr state, int callbackID) { + int ret; + + virDomainEventStateLock(state); if (state->isDispatching) - return virDomainEventCallbackListMarkDeleteID(conn, - state->callbacks, callbackID); + ret = virDomainEventCallbackListMarkDeleteID(conn, + state->callbacks, callbackID); else - return virDomainEventCallbackListRemoveID(conn, - state->callbacks, callbackID); + ret = virDomainEventCallbackListRemoveID(conn, + state->callbacks, callbackID); + virDomainEventStateUnlock(state); + return ret; } diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index b06be16..08930ed 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -61,6 +61,7 @@ struct _virDomainEventState { int timer; /* Flag if we're in process of dispatching */ bool isDispatching; + virMutex lock; }; typedef struct _virDomainEventState virDomainEventState; typedef virDomainEventState *virDomainEventStatePtr; -- 1.7.3.4
On 10/10/2011 05:45 AM, Michal Privoznik wrote:
Currently, push& pop from event queue (both server& client side) rely on lock from higher levels, e.g. on driver lock (qemu), private_data (remote), ...; This alone is not sufficient as not every function that interacts with this queue can/does lock, esp. in client where we have a different approach, "passing the buck".
Therefore we need a separate lock just to protect event queue.
For more info see: https://bugzilla.redhat.com/show_bug.cgi?id=743817 --- src/conf/domain_event.c | 65 +++++++++++++++++++++++++++++++++++++++++------ src/conf/domain_event.h | 1 + 2 files changed, 58 insertions(+), 8 deletions(-)
I'd feel more comfortable if Dan also reviews, but I'll give it a shot.
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 3189346..6cc8168 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -547,6 +547,24 @@ virDomainEventQueuePtr virDomainEventQueueNew(void) return ret; }
+static void +virDomainEventStateLock(virDomainEventStatePtr state) +{ + if (!state) + return; + + virMutexLock(&state->lock);
Why do these have early returns, implying NULL state is okay...
@@ -1161,18 +1188,26 @@ void virDomainEventStateQueue(virDomainEventStatePtr state, virDomainEventPtr event) { + int count; + if (state->timer< 0) { virDomainEventFree(event); return; }
+ virDomainEventStateLock(state); + if (virDomainEventQueuePush(state->queue, event)< 0) { VIR_DEBUG("Error adding event to queue"); virDomainEventFree(event); }
- if (state->queue->count == 1) + count = state->queue->count; + virDomainEventStateUnlock(state);
...even though uses like this blindly assume that state is non-NULL? One of the two places is wrong (I'd argue that the lock/unlock functions should be ATTRIBUTE_NONNULL(1), since it looks like all callers avoid passing NULL). Beyond that, looks okay to me. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Oct 10, 2011 at 01:45:50PM +0200, Michal Privoznik wrote:
Currently, push & pop from event queue (both server & client side) rely on lock from higher levels, e.g. on driver lock (qemu), private_data (remote), ...; This alone is not sufficient as not every function that interacts with this queue can/does lock, esp. in client where we have a different approach, "passing the buck".
Therefore we need a separate lock just to protect event queue.
+static void +virDomainEventStateLock(virDomainEventStatePtr state) +{ + if (!state) + return; + + virMutexLock(&state->lock); +} + +static void +virDomainEventStateUnlock(virDomainEventStatePtr state) +{ + if (!state) + return; + + virMutexUnlock(&state->lock); +}
Normal coding policy is to *not* check for NULL in the mutex functions. It is safer to crash on NULL, then to return without locking.
virDomainEventStateQueue(virDomainEventStatePtr state, virDomainEventPtr event) { + int count; + if (state->timer < 0) { virDomainEventFree(event); return; }
+ virDomainEventStateLock(state); + if (virDomainEventQueuePush(state->queue, event) < 0) { VIR_DEBUG("Error adding event to queue"); virDomainEventFree(event); }
- if (state->queue->count == 1) + count = state->queue->count; + virDomainEventStateUnlock(state); + + if (count == 1) virEventUpdateTimeout(state->timer, 0); + }
Updating the timer outside the lock is unsafe.
void @@ -1182,6 +1217,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state, { virDomainEventQueue tempQueue;
+ virDomainEventStateLock(state); state->isDispatching = true;
/* Copy the queue, so we're reentrant safe when dispatchFunc drops the @@ -1190,6 +1226,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state, tempQueue.events = state->queue->events; state->queue->count = 0; state->queue->events = NULL; + virDomainEventStateUnlock(state);
virEventUpdateTimeout(state->timer, -1); virDomainEventQueueDispatch(&tempQueue,
This is unsafe
@@ -1198,9 +1235,11 @@ virDomainEventStateFlush(virDomainEventStatePtr state, opaque);
/* Purge any deleted callbacks */ + virDomainEventStateLock(state); virDomainEventCallbackListPurgeMarked(state->callbacks);
state->isDispatching = false; + virDomainEventStateUnlock(state); }
With these two timer updates you have a race condition with 2 threads: T1: virDomainEventStateFlush T2: virDomainEventQueue 1. Lock 2. Set count = 0; 3. Unlock 4. lock 5. Save current count value 6. Queue new event 7. unlock 8. virEventUpdateTimer(1); 9. virEventUpdateTimer(-1) Now the timer is disabled, but we have an event pending You need to make sure all timer updates are *inside* the critical section. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Currently, push & pop from event queue (both server & client side) rely on lock from higher levels, e.g. on driver lock (qemu), private_data (remote), ...; This alone is not sufficient as not every function that interacts with this queue can/does lock, esp. in client where we have a different approach, "passing the buck". Therefore we need a separate lock just to protect event queue. For more info see: https://bugzilla.redhat.com/show_bug.cgi?id=743817 --- src/conf/domain_event.c | 54 ++++++++++++++++++++++++++++++++++++++++------- src/conf/domain_event.h | 1 + 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 3189346..f712c34 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -547,6 +547,18 @@ virDomainEventQueuePtr virDomainEventQueueNew(void) return ret; } +static void +virDomainEventStateLock(virDomainEventStatePtr state) +{ + virMutexLock(&state->lock); +} + +static void +virDomainEventStateUnlock(virDomainEventStatePtr state) +{ + virMutexUnlock(&state->lock); +} + /** * virDomainEventStateFree: * @list: virDomainEventStatePtr to free @@ -564,6 +576,8 @@ virDomainEventStateFree(virDomainEventStatePtr state) if (state->timer != -1) virEventRemoveTimeout(state->timer); + + virMutexDestroy(&state->lock); VIR_FREE(state); } @@ -590,6 +604,13 @@ virDomainEventStateNew(virEventTimeoutCallback timeout_cb, goto error; } + if (virMutexInit(&state->lock) < 0) { + virReportSystemError(errno, "%s", + _("unable to initialize state mutex")); + VIR_FREE(state); + goto error; + } + if (VIR_ALLOC(state->callbacks) < 0) { virReportOOMError(); goto error; @@ -1166,6 +1187,8 @@ virDomainEventStateQueue(virDomainEventStatePtr state, return; } + virDomainEventStateLock(state); + if (virDomainEventQueuePush(state->queue, event) < 0) { VIR_DEBUG("Error adding event to queue"); virDomainEventFree(event); @@ -1173,6 +1196,7 @@ virDomainEventStateQueue(virDomainEventStatePtr state, if (state->queue->count == 1) virEventUpdateTimeout(state->timer, 0); + virDomainEventStateUnlock(state); } void @@ -1182,6 +1206,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state, { virDomainEventQueue tempQueue; + virDomainEventStateLock(state); state->isDispatching = true; /* Copy the queue, so we're reentrant safe when dispatchFunc drops the @@ -1190,17 +1215,20 @@ virDomainEventStateFlush(virDomainEventStatePtr state, tempQueue.events = state->queue->events; state->queue->count = 0; state->queue->events = NULL; - virEventUpdateTimeout(state->timer, -1); + virDomainEventStateUnlock(state); + virDomainEventQueueDispatch(&tempQueue, state->callbacks, dispatchFunc, opaque); /* Purge any deleted callbacks */ + virDomainEventStateLock(state); virDomainEventCallbackListPurgeMarked(state->callbacks); state->isDispatching = false; + virDomainEventStateUnlock(state); } int @@ -1208,11 +1236,16 @@ virDomainEventStateDeregister(virConnectPtr conn, virDomainEventStatePtr state, virConnectDomainEventCallback callback) { + int ret; + + virDomainEventStateLock(state); if (state->isDispatching) - return virDomainEventCallbackListMarkDelete(conn, - state->callbacks, callback); + ret = virDomainEventCallbackListMarkDelete(conn, + state->callbacks, callback); else - return virDomainEventCallbackListRemove(conn, state->callbacks, callback); + ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback); + virDomainEventStateUnlock(state); + return ret; } int @@ -1220,10 +1253,15 @@ virDomainEventStateDeregisterAny(virConnectPtr conn, virDomainEventStatePtr state, int callbackID) { + int ret; + + virDomainEventStateLock(state); if (state->isDispatching) - return virDomainEventCallbackListMarkDeleteID(conn, - state->callbacks, callbackID); + ret = virDomainEventCallbackListMarkDeleteID(conn, + state->callbacks, callbackID); else - return virDomainEventCallbackListRemoveID(conn, - state->callbacks, callbackID); + ret = virDomainEventCallbackListRemoveID(conn, + state->callbacks, callbackID); + virDomainEventStateUnlock(state); + return ret; } diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index b06be16..08930ed 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -61,6 +61,7 @@ struct _virDomainEventState { int timer; /* Flag if we're in process of dispatching */ bool isDispatching; + virMutex lock; }; typedef struct _virDomainEventState virDomainEventState; typedef virDomainEventState *virDomainEventStatePtr; -- 1.7.3.4
On Wed, Oct 12, 2011 at 01:58:46PM +0200, Michal Privoznik wrote:
Currently, push & pop from event queue (both server & client side) rely on lock from higher levels, e.g. on driver lock (qemu), private_data (remote), ...; This alone is not sufficient as not every function that interacts with this queue can/does lock, esp. in client where we have a different approach, "passing the buck".
Therefore we need a separate lock just to protect event queue.
For more info see: https://bugzilla.redhat.com/show_bug.cgi?id=743817 --- src/conf/domain_event.c | 54 ++++++++++++++++++++++++++++++++++++++++------- src/conf/domain_event.h | 1 + 2 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 3189346..f712c34 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -547,6 +547,18 @@ virDomainEventQueuePtr virDomainEventQueueNew(void) return ret; }
+static void +virDomainEventStateLock(virDomainEventStatePtr state) +{ + virMutexLock(&state->lock); +} + +static void +virDomainEventStateUnlock(virDomainEventStatePtr state) +{ + virMutexUnlock(&state->lock); +} + /** * virDomainEventStateFree: * @list: virDomainEventStatePtr to free @@ -564,6 +576,8 @@ virDomainEventStateFree(virDomainEventStatePtr state)
if (state->timer != -1) virEventRemoveTimeout(state->timer); + + virMutexDestroy(&state->lock); VIR_FREE(state); }
@@ -590,6 +604,13 @@ virDomainEventStateNew(virEventTimeoutCallback timeout_cb, goto error; }
+ if (virMutexInit(&state->lock) < 0) { + virReportSystemError(errno, "%s", + _("unable to initialize state mutex")); + VIR_FREE(state); + goto error; + } + if (VIR_ALLOC(state->callbacks) < 0) { virReportOOMError(); goto error; @@ -1166,6 +1187,8 @@ virDomainEventStateQueue(virDomainEventStatePtr state, return; }
+ virDomainEventStateLock(state); + if (virDomainEventQueuePush(state->queue, event) < 0) { VIR_DEBUG("Error adding event to queue"); virDomainEventFree(event); @@ -1173,6 +1196,7 @@ virDomainEventStateQueue(virDomainEventStatePtr state,
if (state->queue->count == 1) virEventUpdateTimeout(state->timer, 0); + virDomainEventStateUnlock(state); }
void @@ -1182,6 +1206,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state, { virDomainEventQueue tempQueue;
+ virDomainEventStateLock(state); state->isDispatching = true;
/* Copy the queue, so we're reentrant safe when dispatchFunc drops the @@ -1190,17 +1215,20 @@ virDomainEventStateFlush(virDomainEventStatePtr state, tempQueue.events = state->queue->events; state->queue->count = 0; state->queue->events = NULL; - virEventUpdateTimeout(state->timer, -1); + virDomainEventStateUnlock(state); + virDomainEventQueueDispatch(&tempQueue, state->callbacks, dispatchFunc, opaque);
/* Purge any deleted callbacks */ + virDomainEventStateLock(state); virDomainEventCallbackListPurgeMarked(state->callbacks);
state->isDispatching = false; + virDomainEventStateUnlock(state); }
int @@ -1208,11 +1236,16 @@ virDomainEventStateDeregister(virConnectPtr conn, virDomainEventStatePtr state, virConnectDomainEventCallback callback) { + int ret; + + virDomainEventStateLock(state); if (state->isDispatching) - return virDomainEventCallbackListMarkDelete(conn, - state->callbacks, callback); + ret = virDomainEventCallbackListMarkDelete(conn, + state->callbacks, callback); else - return virDomainEventCallbackListRemove(conn, state->callbacks, callback); + ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback); + virDomainEventStateUnlock(state); + return ret; }
int @@ -1220,10 +1253,15 @@ virDomainEventStateDeregisterAny(virConnectPtr conn, virDomainEventStatePtr state, int callbackID) { + int ret; + + virDomainEventStateLock(state); if (state->isDispatching) - return virDomainEventCallbackListMarkDeleteID(conn, - state->callbacks, callbackID); + ret = virDomainEventCallbackListMarkDeleteID(conn, + state->callbacks, callbackID); else - return virDomainEventCallbackListRemoveID(conn, - state->callbacks, callbackID); + ret = virDomainEventCallbackListRemoveID(conn, + state->callbacks, callbackID); + virDomainEventStateUnlock(state); + return ret; } diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index b06be16..08930ed 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -61,6 +61,7 @@ struct _virDomainEventState { int timer; /* Flag if we're in process of dispatching */ bool isDispatching; + virMutex lock; }; typedef struct _virDomainEventState virDomainEventState; typedef virDomainEventState *virDomainEventStatePtr;
ACK, the locking is correct now. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12.10.2011 22:08, Daniel P. Berrange wrote:
On Wed, Oct 12, 2011 at 01:58:46PM +0200, Michal Privoznik wrote:
Currently, push & pop from event queue (both server & client side) rely on lock from higher levels, e.g. on driver lock (qemu), private_data (remote), ...; This alone is not sufficient as not every function that interacts with this queue can/does lock, esp. in client where we have a different approach, "passing the buck".
Therefore we need a separate lock just to protect event queue.
For more info see: https://bugzilla.redhat.com/show_bug.cgi?id=743817 --- src/conf/domain_event.c | 54 ++++++++++++++++++++++++++++++++++++++++------- src/conf/domain_event.h | 1 + 2 files changed, 47 insertions(+), 8 deletions(-)
ACK, the locking is correct now.
Daniel
Thanks, pushed.
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik