[libvirt] [PATCH] events: Propose a separate lock for event queue

[Daniel P. Berrange]

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