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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list