Re: [Xen-devel] [PATCH 00/12] libxl: fork: SIGCHLD flexibility

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH 00/12] libxl: fork: SIGCHLD 
flexibility"):
> Looking at libvirt's default event loop impl, and the current libxl
> driver code, I think this is how things are :-/.  But maybe you have
> just described a bug in the libxl driver.

I think this is a bug in the libvirt timeout system.  At the very
least it should avoid entering the same timeout callback in multiple
threads.

Something like the attached (UNTESTED).

I'm currently working on a test case to demonstrate the fd race.

Ian.


Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 8a4c8bc..41b012d 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -66,6 +66,7 @@ struct virEventPollTimeout {
     virFreeCallback ff;
     void *opaque;
     int deleted;
+    bool in_callback;
 };
 
 /* Allocate extra slots for virEventPollHandle/virEventPollTimeout
@@ -238,6 +239,7 @@ int virEventPollAddTimeout(int frequency,
     eventLoop.timeouts[eventLoop.timeoutsCount].deleted = 0;
     eventLoop.timeouts[eventLoop.timeoutsCount].expiresAt =
         frequency >= 0 ? frequency + now : 0;
+    eventLoop.timeouts[eventLoop.timeoutsCount].in_callback = 0;
 
     eventLoop.timeoutsCount++;
     ret = nextTimer-1;
@@ -334,6 +336,8 @@ static int virEventPollCalculateTimeout(int *timeout) {
     for (i = 0; i < eventLoop.timeoutsCount; i++) {
         if (eventLoop.timeouts[i].deleted)
             continue;
+        if (eventLoop.timeouts[i].in_callback)
+            continue;
         if (eventLoop.timeouts[i].frequency < 0)
             continue;
 
@@ -429,7 +433,9 @@ static int virEventPollDispatchTimeouts(void)
         return -1;
 
     for (i = 0; i < ntimeouts; i++) {
-        if (eventLoop.timeouts[i].deleted || eventLoop.timeouts[i].frequency < 
0)
+        if (eventLoop.timeouts[i].deleted)
+            continue;
+        if (eventLoop.timeouts[i].frequency < 0)
             continue;
 
         /* Add 20ms fuzz so we don't pointlessly spin doing
@@ -447,9 +453,11 @@ static int virEventPollDispatchTimeouts(void)
             PROBE(EVENT_POLL_DISPATCH_TIMEOUT,
                   "timer=%d",
                   timer);
+            eventLoop.timeouts[i].in_callback = 1;
             virMutexUnlock(&eventLoop.lock);
             (cb)(timer, opaque);
             virMutexLock(&eventLoop.lock);
+            eventLoop.timeouts[i].in_callback = 0;
         }
     }
     return 0;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel