[Xen-devel] [PATCH 2/2] libxl: Cope with pipes which signal POLLHUP|POLLIN on read eof

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Some operating systems (including Linux and FreeBSD[1]) signal not
(only) POLLIN when a reading pipe reaches EOF, but POLLHUP (with or
without POLLIN).  This is permitted[2].  The implications are that in
the general case it is not possible to determine whether POLLHUP
indicates an error or simply eof without attempting a read.

Datacopiers mishandle this, because they always treat POLLHUP
exceptionally (either reporting it via callback_pollhup, or treating
it as an error).  datacopiers reading from pipes on such OSs can fail
(perhaps leaving some data unprocessed) rather than completing
successfully.

[1] http://www.greenend.org.uk/rjk/tech/poll.html
[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html

Distinguishing POLLHUP is needed for pty fds, but most callers in
libxl do not care about POLLHUP except as an error or eof condition.

So change the datacopier semantics so that if callback_pollhup is not
specified we treat POLLHUP almost like POLLIN.  The difference is that
if we get HUP from poll, but EWOULDBLOCK from read, we must signal an
error ratehr than attempting the read again.

This fixes the problem which 7e9ec50b0535 was aimed at.

Signed-off-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CC: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
---
v2: Take a completely different approach which is correct for OSes
    which always signal pipe EOF with POLLHUP.
---
 tools/libxl/libxl_aoutils.c  |   19 +++++++++++++++----
 tools/libxl/libxl_internal.h |    3 ++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index ddbe6ae..ef679dd 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -208,13 +208,14 @@ static void datacopier_readable(libxl__egc *egc, 
libxl__ev_fd *ev,
     if (datacopier_pollhup_handled(egc, dc, revents, 0))
         return;
 
-    if (revents & ~POLLIN) {
-        LOG(ERROR, "unexpected poll event 0x%x (should be POLLIN)"
+    if (revents & ~(POLLIN|POLLHUP)) {
+        LOG(ERROR,
+            "unexpected poll event 0x%x (expected POLLIN and/or POLLHUP)"
             " on %s during copy of %s", revents, dc->readwhat, dc->copywhat);
         datacopier_callback(egc, dc, -1, 0);
         return;
     }
-    assert(revents & POLLIN);
+    assert(revents & (POLLIN|POLLHUP));
     for (;;) {
         libxl__datacopier_buf *buf = NULL;
         int r;
@@ -243,7 +244,17 @@ static void datacopier_readable(libxl__egc *egc, 
libxl__ev_fd *ev,
         }
         if (r < 0) {
             if (errno == EINTR) continue;
-            if (errno == EWOULDBLOCK) break;
+            if (errno == EWOULDBLOCK) {
+                if (revents & POLLHUP) {
+                    LOG(ERROR,
+                        "poll reported HUP but fd read gave EWOULDBLOCK"
+                        " on %s during copy of %s",
+                        dc->readwhat, dc->copywhat);
+                    datacopier_callback(egc, dc, -1, 0);
+                    return;
+                }
+                break;
+            }
             LOGE(ERROR, "error reading %s during copy of %s",
                  dc->readwhat, dc->copywhat);
             datacopier_callback(egc, dc, 0, errno);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3aba221..b6aa962 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2538,7 +2538,8 @@ typedef struct libxl__datacopier_buf 
libxl__datacopier_buf;
  *     errnoval!=0 means we had a read error, logged
  * onwrite==-1 means some other internal failure, errnoval not valid, logged
  * If we get POLLHUP, we call callback_pollhup(..., onwrite, -1);
- * or if callback_pollhup==0 this is an internal failure, as above.
+ * or if callback_pollhup==0 this is treated as eof (if POLLIN|POLLHUP
+ * on the reading fd) or an internal failure (otherwise), as above.
  * In all cases copier is killed before calling this callback */
 typedef void libxl__datacopier_callback(libxl__egc *egc,
      libxl__datacopier_state *dc, int onwrite, int errnoval);
-- 
1.7.10.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel