[libvirt] [PATCH] virCommand: Properly handle POLLHUP

[Daniel P. Berrange]

On Tue, Jan 03, 2012 at 06:58:51PM +0100, Michal Privoznik wrote:
> It is a good practise to set revents to zero before doing any poll().
> Moreover, we should check if event we waited for really occurred or
> if any of fds we were polling on didn't encountered hangup.
> ---
>  src/util/command.c |   21 +++++++++++++++++----
>  1 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/command.c b/src/util/command.c
> index f5effdf..9b553f0 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -1620,16 +1620,19 @@ virCommandProcessIO(virCommandPtr cmd)
>          if (infd != -1) {
>              fds[nfds].fd = infd;
>              fds[nfds].events = POLLOUT;
> +            fds[nfds].revents = 0;
>              nfds++;
>          }
>          if (outfd != -1) {
>              fds[nfds].fd = outfd;
>              fds[nfds].events = POLLIN;
> +            fds[nfds].revents = 0;
>              nfds++;
>          }
>          if (errfd != -1) {
>              fds[nfds].fd = errfd;
>              fds[nfds].events = POLLIN;
> +            fds[nfds].revents = 0;
>              nfds++;
>          }

ACK to this bit, clearly addressing a bug


>  
> @@ -1645,8 +1648,8 @@ virCommandProcessIO(virCommandPtr cmd)
>          }
>  
>          for (i = 0; i < nfds ; i++) {
> -            if (fds[i].fd == errfd ||
> -                fds[i].fd == outfd) {
> +            if (fds[i].revents & POLLIN &&
> +                (fds[i].fd == errfd || fds[i].fd == outfd)) {
>                  char data[1024];
>                  char **buf;
>                  size_t *len;
> @@ -1684,7 +1687,7 @@ virCommandProcessIO(virCommandPtr cmd)
>                      memcpy(*buf + *len, data, done);
>                      *len += done;
>                  }
> -            } else {
> +            } else if (fds[i].revents & POLLOUT) {
>                  int done;
>  
>                  /* Coverity 5.3.0 can't see that we only get here if
> @@ -1708,8 +1711,18 @@ virCommandProcessIO(virCommandPtr cmd)
>                              VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
>                      }
>                  }
> +            } else if (fds[i].revents & POLLHUP) {
> +                if (fds[i].fd == errfd) {
> +                    VIR_DEBUG("hangup on stderr");
> +                    errfd = -1;
> +                } else if (fds[i].fd == outfd) {
> +                    VIR_DEBUG("hangup on stdout");
> +                    outfd = -1;
> +                } else {
> +                    VIR_DEBUG("hangup on stdin");
> +                    infd = -1;
> +                }
>              }


Was this part of the change actually solving anything ?  When you have
POLLIN set in the events, you'll never get a bare POLLHUP event back
in revents. You'll always get revents set to POLLIN|POLLHUP. Thus since
an earlier if() checks for POLLIN, I'm fairly sure this POLLHUP block
you're adding won't ever run.

If we want to explicitly check events for comprehensively, then I
think we'd want to avoid the if/elseif/elseif style, and use separate
if/if/if tests for each POLLXXX bit.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|