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