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 :|