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

12 Oct 2011

On Wed, Oct 12, 2011 at 01:58:46PM +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.
For more info see:
https://bugzilla.redhat.com/show_bug.cgi?id=743817
---
 src/conf/domain_event.c |   54 ++++++++++++++++++++++++++++++++++++++++-------
 src/conf/domain_event.h |    1 +
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 3189346..f712c34 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -547,6 +547,18 @@ virDomainEventQueuePtr virDomainEventQueueNew(void)
     return ret;
 }
+static void
+virDomainEventStateLock(virDomainEventStatePtr state)
+{
+    virMutexLock(&state->lock);
+}
+
+static void
+virDomainEventStateUnlock(virDomainEventStatePtr state)
+{
+    virMutexUnlock(&state->lock);
+}
+
 /**
  * virDomainEventStateFree:
  * @list: virDomainEventStatePtr to free
@@ -564,6 +576,8 @@ virDomainEventStateFree(virDomainEventStatePtr state)
if (state->timer != -1)
         virEventRemoveTimeout(state->timer);
+
+    virMutexDestroy(&state->lock);
     VIR_FREE(state);
 }
@@ -590,6 +604,13 @@ virDomainEventStateNew(virEventTimeoutCallback timeout_cb,
         goto error;
     }
+    if (virMutexInit(&state->lock) < 0) {
+        virReportSystemError(errno, "%s",
+                             _("unable to initialize state mutex"));
+        VIR_FREE(state);
+        goto error;
+    }
+
     if (VIR_ALLOC(state->callbacks) < 0) {
         virReportOOMError();
         goto error;
@@ -1166,6 +1187,8 @@ virDomainEventStateQueue(virDomainEventStatePtr state,
         return;
     }
+    virDomainEventStateLock(state);
+
     if (virDomainEventQueuePush(state->queue, event) < 0) {
         VIR_DEBUG("Error adding event to queue");
         virDomainEventFree(event);
@@ -1173,6 +1196,7 @@ virDomainEventStateQueue(virDomainEventStatePtr state,
if (state->queue->count == 1)
         virEventUpdateTimeout(state->timer, 0);
+    virDomainEventStateUnlock(state);
 }
void
@@ -1182,6 +1206,7 @@ virDomainEventStateFlush(virDomainEventStatePtr state,
 {
     virDomainEventQueue tempQueue;
+    virDomainEventStateLock(state);
     state->isDispatching = true;
/* Copy the queue, so we're reentrant safe when dispatchFunc drops the
@@ -1190,17 +1215,20 @@ virDomainEventStateFlush(virDomainEventStatePtr state,
     tempQueue.events = state->queue->events;
     state->queue->count = 0;
     state->queue->events = NULL;
-
     virEventUpdateTimeout(state->timer, -1);
+    virDomainEventStateUnlock(state);
+
     virDomainEventQueueDispatch(&tempQueue,
                                 state->callbacks,
                                 dispatchFunc,
                                 opaque);
/* Purge any deleted callbacks */
+    virDomainEventStateLock(state);
     virDomainEventCallbackListPurgeMarked(state->callbacks);
state->isDispatching = false;
+    virDomainEventStateUnlock(state);
 }
int
@@ -1208,11 +1236,16 @@ virDomainEventStateDeregister(virConnectPtr conn,
                               virDomainEventStatePtr state,
                               virConnectDomainEventCallback callback)
 {
+    int ret;
+
+    virDomainEventStateLock(state);
     if (state->isDispatching)
-        return virDomainEventCallbackListMarkDelete(conn,
-                                                    state->callbacks, callback);
+        ret = virDomainEventCallbackListMarkDelete(conn,
+                                                   state->callbacks, callback);
     else
-        return virDomainEventCallbackListRemove(conn, state->callbacks, callback);
+        ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback);
+    virDomainEventStateUnlock(state);
+    return ret;
 }
int
@@ -1220,10 +1253,15 @@ virDomainEventStateDeregisterAny(virConnectPtr conn,
                                  virDomainEventStatePtr state,
                                  int callbackID)
 {
+    int ret;
+
+    virDomainEventStateLock(state);
     if (state->isDispatching)
-        return virDomainEventCallbackListMarkDeleteID(conn,
-                                                      state->callbacks, callbackID);
+        ret = virDomainEventCallbackListMarkDeleteID(conn,
+                                                     state->callbacks, callbackID);
     else
-        return virDomainEventCallbackListRemoveID(conn,
-                                                  state->callbacks, callbackID);
+        ret = virDomainEventCallbackListRemoveID(conn,
+                                                 state->callbacks, callbackID);
+    virDomainEventStateUnlock(state);
+    return ret;
 }
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
index b06be16..08930ed 100644
--- a/src/conf/domain_event.h
+++ b/src/conf/domain_event.h
@@ -61,6 +61,7 @@ struct _virDomainEventState {
     int timer;
     /* Flag if we're in process of dispatching */
     bool isDispatching;
+    virMutex lock;
 };
 typedef struct _virDomainEventState virDomainEventState;
 typedef virDomainEventState *virDomainEventStatePtr;
ACK, the locking is correct now.


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

    

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

Daniel P. Berrange