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 :|