[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]