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

[Jiri Denemark]

On Mon, Mar 07, 2011 at 14:18:06 +0100, Jiri Denemark wrote:
> On Mon, Mar 07, 2011 at 14:13:23 +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.
> 
> Eh, my fingers were faster than my brain :-) Dispatch* are safe since they do
> anything with the old pointer after getting back from a callback.

And the rest of the for loops in Cleanup* is safe as well for the reason you
already mentioned. The code moves stuff from i+1 to the end of the array one
position closer the beginning of the array and it will move the possibly added
items as well. The only thing which may happen is that if Remove* is called
while the mutex is unlocked, an item which we already went through can be
marked as deleted so we are no longer guaranteed that when Cleanup* finishes,
the array only contains items which were not deleted. But AFAICT no-one counts
with that behavior so it's safe to break it.

Sorry for the noise.

Jirka