Re: [libvirt] PATCH: 25/28: Thread safety for libvirtd event loop

On Mon, Dec 01, 2008 at 12:18:24AM +0000, Daniel P. Berrange wrote: > This patch makes the event loop in the libvirtd daemon thread safe, and > re-entrant. This allows one thread to add/remove/update timers/file handle > watches while other thread is doing the poll. This sometimes requires that > we wakeup the main thread to make it see changes to the poll FD list. We > use the traditional self-pipe trick for this task. [...] > -static int virEventDispatchHandles(struct pollfd *fds) { > +static int virEventDispatchHandles(int nfds, struct pollfd *fds) { > int i; > - virEventHandleType hEvents; > - /* Save this now - it may be changed during dispatch */ > - int nhandles = eventLoop.handlesCount; > > - for (i = 0 ; i < nhandles ; i++) { > + for (i = 0 ; i < nfds ; i++) { > if (eventLoop.handles[i].deleted) { > EVENT_DEBUG("Skip deleted %d", eventLoop.handles[i].fd); > continue; > } > > if (fds[i].revents) { > - hEvents = virPollEventToEventHandleType(fds[i].revents); > - EVENT_DEBUG("Dispatch %d %d %d %p", > - eventLoop.handles[i].watch, > - fds[i].fd, fds[i].revents, > - eventLoop.handles[i].opaque); > - (eventLoop.handles[i].cb)(eventLoop.handles[i].watch, > - fds[i].fd, > - hEvents, > - eventLoop.handles[i].opaque); > + virEventHandleCallback cb = eventLoop.handles[i].cb; > + void *opaque = eventLoop.handles[i].opaque; > + int hEvents = virPollEventToEventHandleType(fds[i].revents); > + EVENT_DEBUG("Dispatch %d %d %p", fds[i].fd, > + fds[i].revents, eventLoop.handles[i].opaque); > + virEventUnlock(); > + (cb)(eventLoop.handles[i].watch, > + fds[i].fd, hEvents, opaque); > + virEventLock(); > } > } Hum, if this routine assume we enter it with the event lock and keep it though its lifetime except when dispatching, then I guess it's worth a comment, right ? Is there only one thread which can access or unregister fds, or do we have another lock in place ?