commit 06b9c5b9231ef4dbd4b5ff69564305cd4f814879 Author: Michal Privoznik Date: Tue Jan 3 18:40:55 2012 +0100 virCommand: Properly handle POLLHUP 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. diff --git a/src/util/command.c b/src/util/command.c index f5effdf..bdaa88b 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++; } @@ -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,10 @@ virCommandProcessIO(virCommandPtr cmd) memcpy(*buf + *len, data, done); *len += done; } - } else { + } + + if (fds[i].revents & POLLOUT && + fds[i].fd == infd) { int done; /* Coverity 5.3.0 can't see that we only get here if @@ -1710,6 +1716,18 @@ virCommandProcessIO(virCommandPtr cmd) } } + if (fds[i].revents & (POLLHUP | POLLERR)) { + 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; + } + } } } commit d19149dda888d36cea58b6cdf7446f98bd1bf734 Author: Laszlo Ersek Date: Tue Jan 24 15:55:19 2012 +0100 virCommandProcessIO(): make poll() usage more robust POLLIN and POLLHUP are not mutually exclusive. Currently the following seems possible: the child writes 3K to its stdout or stderr pipe, and immediately closes it. We get POLLIN|POLLHUP (I'm not sure that's possible on Linux, but SUSv4 seems to allow it). We read 1K and throw away the rest. When poll() returns and we're about to check the /revents/ member in a given array element, let's map all the revents bits to two (independent) ideas: "let's attempt to read()", and "let's attempt to write()". This should cover all errors, EOFs, and normal conditions; the read()/write() call should report any pending error. Under this approach, both POLLHUP and POLLERR are mapped to "needs read()" if we're otherwise prepared for POLLIN. POLLERR also maps to "needs write()" if we're otherwise prepared for POLLOUT. The rest of the mappings (POLLPRI etc.) would be easy, but probably useless for pipes. Additionally, SUSv4 doesn't appear to forbid POLLIN|POLLERR (or POLLOUT|POLLERR) set simultaneously. One could argue that the read() or write() call would return without blocking in these cases (with an error), so POLLIN / POLLOUT would be justified beside POLLERR. The code now penalizes POLLIN|POLLERR differently from plain POLLERR. The former (ie. read() returning -1) is terminal and we jump to cleanup, while plain POLLERR masks only the affected file descriptor for the future. Let's unify those. Signed-off-by: Laszlo Ersek diff --git a/src/util/command.c b/src/util/command.c index f05493e..dc3cfc5 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1710,7 +1710,7 @@ virCommandProcessIO(virCommandPtr cmd) } for (i = 0; i < nfds ; i++) { - if (fds[i].revents & POLLIN && + if (fds[i].revents & (POLLIN | POLLHUP | POLLERR) && (fds[i].fd == errfd || fds[i].fd == outfd)) { char data[1024]; char **buf; @@ -1751,7 +1751,7 @@ virCommandProcessIO(virCommandPtr cmd) } } - if (fds[i].revents & POLLOUT && + if (fds[i].revents & (POLLOUT | POLLERR) && fds[i].fd == infd) { int done; @@ -1777,19 +1777,6 @@ virCommandProcessIO(virCommandPtr cmd) } } } - - if (fds[i].revents & (POLLHUP | POLLERR)) { - 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; - } - } } }