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 <lersek@redhat.com>

diff --git a/AUTHORS b/AUTHORS
index 74f31577462310aad1aa1d5255f011216751da05..783c48a16ef087f7a6df5d49d66e3898fe479386 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -216,6 +216,7 @@ Patches have also been contributed by:
   Yuri Chornoivan      <yurchor@ukr.net>
   Deepak C Shetty      <deepakcs@linux.vnet.ibm.com>
   Martin Kletzander    <mkletzan@redhat.com>
+  Laszlo Ersek         <lersek@redhat.com>
 
   [....send patches to get your name here....]
 

diff --git a/src/util/command.c b/src/util/command.c
index f05493edfad3904a35e858d4494127f9d931716a..dc3cfc54306c009c3ee69a1e4abbb55bc1f41032 100644 (file)
--- 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;
-                }
-            }
         }
     }