[libvirt] PATCH: 3/4: Fix handling of deleted file handles

[Daniel Veillard]

On Mon, May 11, 2009 at 12:21:40PM +0100, Daniel P. Berrange wrote:
> This patch fixes handling of deleted file handles. First of all it reverts
> the patch http://www.redhat.com/archives/libvir-list/2009-March/msg00246.html
> As an alternative solution to that original problem, the first thing that
> virEventRunOnce() does, is to purge all timers/watches which were marked as
> deleted. This deals with deletes that happen between invocations of the
> virEventRunOnce() method. It also ensures that when going into poll(), all
> the registered timers/watches are live. This guarentees that array indexes 
> match up between the poll FD array and our list of watches. Then during
> the dispatch of FDs, we have a simpler check to skip invocation of file
> handles / timers marked as deleted.
[...]
> -static int virEventMakePollFDs(struct pollfd **retfds) {
> +static struct pollfd *virEventMakePollFDs(void) {
>      struct pollfd *fds;
> -    int i, nfds = 0;
> +    int i;
> +
> +    /* Setup the poll file handle data structs */
> +    if (VIR_ALLOC_N(fds, eventLoop.handlesCount) < 0)
> +        return NULL;
>  
>      for (i = 0 ; i < eventLoop.handlesCount ; i++) {
> -        if (eventLoop.handles[i].deleted)
> -            continue;
> -        nfds++;
> -    }
> -    *retfds = NULL;
> -    /* Setup the poll file handle data structs */
> -    if (VIR_ALLOC_N(fds, nfds) < 0)
> -        return -1;
> -
> -    for (i = 0, nfds = 0 ; i < eventLoop.handlesCount ; i++) {
> -        if (eventLoop.handles[i].deleted)
> -            continue;
> -        fds[nfds].fd = eventLoop.handles[i].fd;
> -        fds[nfds].events = eventLoop.handles[i].events;
> -        fds[nfds].revents = 0;
> +        EVENT_DEBUG("Prepare n=%d w=%d, f=%d e=%d", i,
> +                    eventLoop.handles[i].watch,
> +                    eventLoop.handles[i].fd,
> +                    eventLoop.handles[i].events);
> +        fds[i].fd = eventLoop.handles[i].fd;
> +        fds[i].events = eventLoop.handles[i].events;
> +        fds[i].revents = 0;
>          //EVENT_DEBUG("Wait for %d %d", eventLoop.handles[i].fd, eventLoop.handles[i].events);
> -        nfds++;
>      }
>  
> -    *retfds = fds;
> -    return nfds;
> +    return fds;
>  }

  Okay the loop is way simpler now

>  
> @@ -435,26 +429,30 @@ static int virEventDispatchTimeouts(void
>   * Returns 0 upon success, -1 if an error occurred
>   */
>  static int virEventDispatchHandles(int nfds, struct pollfd *fds) {
> -    int i, n;
> +    int i;
>  
> -    for (i = 0, n = 0 ; i < eventLoop.handlesCount && n < nfds ; i++) {
> +    /* NB, use nfds not eventLoop.handlesCount, because new
> +     * fds might be added on end of list, and they're not
> +     * in the fds array we've got */
> +    for (i = 0 ; i < nfds ; i++) {
>          if (eventLoop.handles[i].deleted) {
> -            EVENT_DEBUG("Skip deleted %d", eventLoop.handles[i].fd);
> +            EVENT_DEBUG("Skip deleted n=%d w=%d f=%d", i,
> +                        eventLoop.handles[i].watch, eventLoop.handles[i].fd);
>              continue;
>          }
>  
> -        if (fds[n].revents) {
> +        if (fds[i].revents) {
>              virEventHandleCallback cb = eventLoop.handles[i].cb;
>              void *opaque = eventLoop.handles[i].opaque;
> -            int hEvents = virPollEventToEventHandleType(fds[n].revents);
> -            EVENT_DEBUG("Dispatch %d %d %p", fds[n].fd,
> -                        fds[n].revents, eventLoop.handles[i].opaque);
> +            int hEvents = virPollEventToEventHandleType(fds[i].revents);
> +            EVENT_DEBUG("Dispatch n=%d f=%d w=%d e=%d %p", i,
> +                        fds[i].fd, eventLoop.handles[i].watch,
> +                        fds[i].revents, eventLoop.handles[i].opaque);
>              virEventUnlock();
>              (cb)(eventLoop.handles[i].watch,
> -                 fds[n].fd, hEvents, opaque);
> +                 fds[i].fd, hEvents, opaque);
>              virEventLock();
>          }
> -        n++;
>      }
>  
>      return 0;

  and here too

> @@ -545,22 +543,21 @@ static int virEventCleanupHandles(void) 
>   * at least one file handle has an event, or a timer expires
>   */
>  int virEventRunOnce(void) {
> -    struct pollfd *fds;
> +    struct pollfd *fds = NULL;
>      int ret, timeout, nfds;
>  
>      virEventLock();
>      eventLoop.running = 1;
>      eventLoop.leader = pthread_self();
> -    if ((nfds = virEventMakePollFDs(&fds)) < 0) {
> -        virEventUnlock();
> -        return -1;
> -    }
>  
> -    if (virEventCalculateTimeout(&timeout) < 0) {
> -        VIR_FREE(fds);
> -        virEventUnlock();
> -        return -1;
> -    }
> +    if (virEventCleanupTimeouts() < 0 ||
> +        virEventCleanupHandles() < 0)
> +        goto error;
> +
> +    if (!(fds = virEventMakePollFDs()) ||
> +        virEventCalculateTimeout(&timeout) < 0)
> +        goto error;
> +    nfds = eventLoop.handlesCount;
>  
>      virEventUnlock();

  here we separate the processing

> @@ -572,38 +569,31 @@ int virEventRunOnce(void) {
>          if (errno == EINTR) {
>              goto retry;
>          }
> -        VIR_FREE(fds);
> -        return -1;
> +        goto error_unlocked;
>      }
>  
>      virEventLock();
> -    if (virEventDispatchTimeouts() < 0) {
> -        VIR_FREE(fds);
> -        virEventUnlock();
> -        return -1;
> -    }
> +    if (virEventDispatchTimeouts() < 0)
> +        goto error;
>  
>      if (ret > 0 &&
> -        virEventDispatchHandles(nfds, fds) < 0) {
> -        VIR_FREE(fds);
> -        virEventUnlock();
> -        return -1;
> -    }
> -    VIR_FREE(fds);
> +        virEventDispatchHandles(nfds, fds) < 0)
> +        goto error;
>  
> -    if (virEventCleanupTimeouts() < 0) {
> -        virEventUnlock();
> -        return -1;
> -    }
> -
> -    if (virEventCleanupHandles() < 0) {
> -        virEventUnlock();
> -        return -1;
> -    }
> +    if (virEventCleanupTimeouts() < 0 ||
> +        virEventCleanupHandles() < 0)
> +        goto error;
>  
>      eventLoop.running = 0;
>      virEventUnlock();
> +    VIR_FREE(fds);
>      return 0;
> +
> +error:
> +    virEventUnlock();
> +error_unlocked:
> +    VIR_FREE(fds);
> +    return -1;
>  }

  Okay, looks fine !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/