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

Re: [libvirt] [PATCH] unlock eventLoop before calling callback function



On 03/07/2011 08:17 AM, Daniel P. Berrange wrote:
On Mon, Mar 07, 2011 at 02:13:23PM +0100, Jiri Denemark wrote:
On Mon, Mar 07, 2011 at 11:15:38 +0000, Daniel P. Berrange wrote:
-        if (eventLoop.handles[i].ff)
+        if (eventLoop.handles[i].ff) {
+            virMutexUnlock(&eventLoop.lock);
              (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque);
+            virMutexLock(&eventLoop.lock);
+        }
I'm a little concerned as to whether the rest of the code in
virEventCleanupHandles/CleanupTimeouts is safe, if we release
the lock here. eg, if some other thread calls virEventAddTimeout
of AddHandle, is there any way this could cause problems for us
here. So far I think this is safe because AddTimeout/AddHandle
will simply append to the end of the array we're iterating over,
but would like a second opinion before ACK'ing
I don't think it's safe to unlock eventloop.lock even in the existing
Dispatch{Timeouts,Handles} cases because Add{Timeout,Handle} use realloc which
is allowed to allocate a new array, move the contents of the old one into it
and free the old array. So the for loop can easily end up accessing memory
which has already been freed.
That's a very unlikely scenario, but yes it could happen. We'd need to
save a copy of the row we're accessing. Thus instead of

       if (eventLoop.handles[i].ff) {
           virMutexUnlock(&eventLoop.lock);
           (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque);
           virMutexLock(&eventLoop.lock);
       }

We probably need

       if (eventLoop.handles[i].ff) {
           virFreeCallback ff = eventLoop.handles[i].ff;
           void *opaque = eventLoop.handles[i].opaque;
           virMutexUnlock(&eventLoop.lock);
           ff(opaque);
           virMutexLock(&eventLoop.lock);
       }

I tested this patch on the two cleanup functions with a save/restore of both a transient and a persistent domain, and both completed without problems.

Based on this, and Dan's explanation of the way that the eventLoop lock *should* work, I can ACK a version of Wen Congyang's patch with Dan's suggestion from above.


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