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

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); }