On 01/03/2012 10:58 AM, 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. Looks like this also solves: https://bugzilla.redhat.com/show_bug.cgi?id=770788 > --- > src/util/command.c | 21 +++++++++++++++++---- > 1 files changed, 17 insertions(+), 4 deletions(-) >

Should we also be probing for POLLERR? (POLLNVAL shouldn't ever be present, because it would indicate a bigger bug in our code). ACK - what you have is a strict improvement, even if we further decide to check for POLLERR.

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