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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org