[PATCH 0/6] tools/libxl: Improvements to libxl__datacopier

[PATCH 0/6] tools/libxl: Improvements to libxl__datacopier

[Andrew Cooper]

To support migration v2, we need far more flexibility out of the datacopier.

This series adds the ability to read from an fd into a local buffer and to
copy a specific number of bytes rather than to EOF.  It also contains bugfixes
related to writing from a local buffer, and POLLHUP handling.

Andrew Cooper (2):
  tools/libxl: Introduce min and max macros
  tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

Ross Lagerwall (3):
  tools/libxl: Allow adding larger amounts of prefixdata to datacopier
  tools/libxl: Allow limiting amount copied by datacopier
  tools/libxl: Extend datacopier to support reading into a buffer

Wen Congyang (1):
  tools/libxl: Update datacopier to support sending data only

 tools/libxl/libxl_aoutils.c    |  121 +++++++++++++++++++++++++---------------
 tools/libxl/libxl_bootloader.c |   24 +++++++-
 tools/libxl/libxl_dom.c        |    1 +
 tools/libxl/libxl_internal.h   |   35 ++++++++++--
 4 files changed, 129 insertions(+), 52 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] tools/libxl: Introduce min and max macros

[Andrew Cooper]

This is the same set used by libxc.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.h |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..a2b6fb7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -108,6 +108,22 @@
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
+#define min(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x < _y) ? _x : _y; })
+#define max(X, Y) ({                             \
+            const typeof (X) _x = (X);           \
+            const typeof (Y) _y = (Y);           \
+            (void) (&_x == &_y);                 \
+            (_x > _y) ? _x : _y; })
+
+#define min_t(type,x,y) \
+        ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
+#define max_t(type,x,y) \
+        ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
+
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
-- 
1.7.10.4

[PATCH 2/6] tools/libxl: Update datacopier to support sending data only

[Andrew Cooper]

From: Wen Congyang <wency@cn.fujitsu.com>

datacopier is to read some data and write it out. If we
have some data to send it over network, we cannot use
datacopier. Update it to support this case.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_aoutils.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index b10d2e1..3e0c0ae 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -309,9 +309,11 @@ int libxl__datacopier_start(libxl__datacopier_state *dc)
 
     libxl__datacopier_init(dc);
 
-    rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
-                               dc->readfd, POLLIN);
-    if (rc) goto out;
+    if (dc->readfd >= 0) {
+        rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
+                                   dc->readfd, POLLIN);
+        if (rc) goto out;
+    }
 
     rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
                                dc->writefd, POLLOUT);
-- 
1.7.10.4

[PATCH 3/6] tools/libxl: Allow adding larger amounts of prefixdata to datacopier

[Andrew Cooper]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Previously, adding more than 1000 bytes of data would cause a segfault.
Now, the maximum amount of data that can be added is limited by maxsz.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_aoutils.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 3e0c0ae..6882ca3 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -160,6 +160,8 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
 {
     EGC_GC;
     libxl__datacopier_buf *buf;
+    const uint8_t *ptr;
+
     /*
      * It is safe for this to be called immediately after _start, as
      * is documented in the public comment.  _start's caller must have
@@ -170,12 +172,14 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
 
     assert(len < dc->maxsz - dc->used);
 
-    buf = libxl__zalloc(NOGC, sizeof(*buf));
-    buf->used = len;
-    memcpy(buf->buf, data, len);
+    for (ptr = data; len; len -= buf->used, ptr += buf->used) {
+        buf = libxl__malloc(NOGC, sizeof(*buf));
+        buf->used = min(len, sizeof(buf->buf));
+        memcpy(buf->buf, ptr, buf->used);
 
-    dc->used += len;
-    LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
+        dc->used += buf->used;
+        LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
+    }
 }
 
 static int datacopier_pollhup_handled(libxl__egc *egc,
-- 
1.7.10.4

[PATCH 4/6] tools/libxl: Allow limiting amount copied by datacopier

[Andrew Cooper]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Add a parameter, maxread, to limit the amount of data read from the
source fd of a datacopier.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_aoutils.c    |   11 +++++++----
 tools/libxl/libxl_bootloader.c |    2 ++
 tools/libxl/libxl_dom.c        |    1 +
 tools/libxl/libxl_internal.h   |    1 +
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 6882ca3..037244e 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -145,7 +145,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
                 return;
             }
         }
-    } else if (!libxl__ev_fd_isregistered(&dc->toread)) {
+    } else if (!libxl__ev_fd_isregistered(&dc->toread) || dc->maxread == 0) {
         /* we have had eof */
         datacopier_callback(egc, dc, 0, 0);
         return;
@@ -231,9 +231,8 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
             buf->used = 0;
             LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
         }
-        int r = read(ev->fd,
-                     buf->buf + buf->used,
-                     sizeof(buf->buf) - buf->used);
+        int r = read(ev->fd, buf->buf + buf->used,
+                     min_t(size_t, sizeof(buf->buf) - buf->used, dc->maxread));
         if (r < 0) {
             if (errno == EINTR) continue;
             if (errno == EWOULDBLOCK) break;
@@ -258,7 +257,11 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
         }
         buf->used += r;
         dc->used += r;
+        dc->maxread -= r;
         assert(buf->used <= sizeof(buf->buf));
+        assert(dc->maxread >= 0);
+        if (dc->maxread == 0)
+            break;
     }
     datacopier_check_state(egc, dc);
 }
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 79947d4..1503101 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -516,6 +516,7 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
 
     bl->keystrokes.ao = ao;
     bl->keystrokes.maxsz = BOOTLOADER_BUF_OUT;
+    bl->keystrokes.maxread = INT_MAX;
     bl->keystrokes.copywhat =
         GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
     bl->keystrokes.callback =         bootloader_keystrokes_copyfail;
@@ -527,6 +528,7 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
 
     bl->display.ao = ao;
     bl->display.maxsz = BOOTLOADER_BUF_IN;
+    bl->display.maxread = INT_MAX;
     bl->display.copywhat =
         GCSPRINTF("bootloader output for domain %"PRIu32, bl->domid);
     bl->display.callback =         bootloader_display_copyfail;
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 48d661a..2c8943b 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1922,6 +1922,7 @@ void libxl__domain_save_device_model(libxl__egc *egc,
     dc->readfd = -1;
     dc->writefd = fd;
     dc->maxsz = INT_MAX;
+    dc->maxread = INT_MAX;
     dc->copywhat = GCSPRINTF("qemu save file for domain %"PRIu32, dss->domid);
     dc->writewhat = "save/migration stream";
     dc->callback = save_device_model_datacopier_done;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a2b6fb7..b7fd13d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2545,6 +2545,7 @@ struct libxl__datacopier_state {
     libxl__ao *ao;
     int readfd, writefd;
     ssize_t maxsz;
+    ssize_t maxread;
     const char *copywhat, *readwhat, *writewhat; /* for error msgs */
     FILE *log; /* gets a copy of everything */
     libxl__datacopier_callback *callback;
-- 
1.7.10.4

[PATCH 5/6] tools/libxl: Extend datacopier to support reading into a buffer

[Andrew Cooper]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Implement a read-only mode for libxl__datacopier.  The mode is invoked
when readbuf is set and writefd is < 0.  On success, the callback passes
the number of bytes read.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_aoutils.c  |   58 +++++++++++++++++++++++++-----------------
 tools/libxl/libxl_internal.h |    4 ++-
 2 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 037244e..a402e5c 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -134,7 +134,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
     STATE_AO_GC(dc->ao);
     int rc;
     
-    if (dc->used) {
+    if (dc->used && !dc->readbuf) {
         if (!libxl__ev_fd_isregistered(&dc->towrite)) {
             rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
                                        dc->writefd, POLLOUT);
@@ -147,7 +147,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
         }
     } else if (!libxl__ev_fd_isregistered(&dc->toread) || dc->maxread == 0) {
         /* we have had eof */
-        datacopier_callback(egc, dc, 0, 0);
+        datacopier_callback(egc, dc, 0, dc->readbuf ? dc->used : 0);
         return;
     } else {
         /* nothing buffered, but still reading */
@@ -215,24 +215,30 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
     }
     assert(revents & POLLIN);
     for (;;) {
-        while (dc->used >= dc->maxsz) {
-            libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
-            dc->used -= rm->used;
-            assert(dc->used >= 0);
-            LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
-            free(rm);
-        }
+        libxl__datacopier_buf *buf = NULL;
+        int r;
+
+        if (dc->readbuf) {
+            r = read(ev->fd, dc->readbuf + dc->used, dc->maxread);
+        } else {
+            while (dc->used >= dc->maxsz) {
+                libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
+                dc->used -= rm->used;
+                assert(dc->used >= 0);
+                LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
+                free(rm);
+            }
 
-        libxl__datacopier_buf *buf =
-            LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
-        if (!buf || buf->used >= sizeof(buf->buf)) {
-            buf = malloc(sizeof(*buf));
-            if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
-            buf->used = 0;
-            LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
-        }
-        int r = read(ev->fd, buf->buf + buf->used,
+            buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
+            if (!buf || buf->used >= sizeof(buf->buf)) {
+                buf = malloc(sizeof(*buf));
+                if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
+                buf->used = 0;
+                LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
+            }
+            r = read(ev->fd, buf->buf + buf->used,
                      min_t(size_t, sizeof(buf->buf) - buf->used, dc->maxread));
+        }
         if (r < 0) {
             if (errno == EINTR) continue;
             if (errno == EWOULDBLOCK) break;
@@ -255,10 +261,12 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
                 return;
             }
         }
-        buf->used += r;
+        if (!dc->readbuf) {
+            buf->used += r;
+            assert(buf->used <= sizeof(buf->buf));
+        }
         dc->used += r;
         dc->maxread -= r;
-        assert(buf->used <= sizeof(buf->buf));
         assert(dc->maxread >= 0);
         if (dc->maxread == 0)
             break;
@@ -316,15 +324,19 @@ int libxl__datacopier_start(libxl__datacopier_state *dc)
 
     libxl__datacopier_init(dc);
 
+    assert(dc->readfd >= 0 || dc->writefd >= 0);
+
     if (dc->readfd >= 0) {
         rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
                                    dc->readfd, POLLIN);
         if (rc) goto out;
     }
 
-    rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
-                               dc->writefd, POLLOUT);
-    if (rc) goto out;
+    if (dc->writefd >= 0) {
+        rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
+                                   dc->writefd, POLLOUT);
+        if (rc) goto out;
+    }
 
     return 0;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b7fd13d..c2e7aa4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2524,7 +2524,8 @@ _hidden void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
 
 /* onwrite==1 means failure happened when writing, logged, errnoval is valid
  * onwrite==0 means failure happened when reading
- *     errnoval==0 means we got eof and all data was written
+ *     errnoval>=0 means we got eof and all data was written or number of bytes
+ *                 written when in read mode
  *     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);
@@ -2553,6 +2554,7 @@ struct libxl__datacopier_state {
     /* remaining fields are private to datacopier */
     libxl__ev_fd toread, towrite;
     ssize_t used;
+    void *readbuf;
     LIBXL_TAILQ_HEAD(libxl__datacopier_bufs, libxl__datacopier_buf) bufs;
 };
 
-- 
1.7.10.4

[PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

[Andrew Cooper]

POLLIN|POLLHUP is a valid revent to recieve from poll() when the far end has
hung up, but data is still available to be read.

Currently all POLLHUPs are fatal.  This is a problem when using the legacy
stream conversion script which will exit 0 when it has completed its task and
has left valid data in the pipe.  In this case, libxl wishes to read until
eof, rather than dying because the conversion script exited.

Adjustments are as follows:
 1. The datacopier pollhup callback changes type to include a return value.
 2. datacopier_handle_pollhup() takes revents by pointer, and calls the
    datacopier pollhup callback ahead of killing the datacopier.
 3. The callback may now indicate that the POLLHUP is not fatal, in which case
    datacopier_handle_pollhup() mask POLLHUP out of revents, and
    datacopier_{read,write}able() allowed to proceed as before.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_aoutils.c    |   34 ++++++++++++++++++++++------------
 tools/libxl/libxl_bootloader.c |   22 ++++++++++++++++++++--
 tools/libxl/libxl_internal.h   |   14 ++++++++++----
 3 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index a402e5c..f3e5f98 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -182,21 +182,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
     }
 }
 
-static int datacopier_pollhup_handled(libxl__egc *egc,
+/*
+ * Handle a POLLHUP signal on a datacopier fd.  Return boolean indicating
+ * whether further processing should cease.
+ */
+static bool datacopier_handle_pollhup(libxl__egc *egc,
                                       libxl__datacopier_state *dc,
-                                      short revents, int onwrite)
+                                      short *revents, int onwrite)
 {
     STATE_AO_GC(dc->ao);
 
-    if (dc->callback_pollhup && (revents & POLLHUP)) {
-        LOG(DEBUG, "received POLLHUP on %s during copy of %s",
-            onwrite ? dc->writewhat : dc->readwhat,
-            dc->copywhat);
-        libxl__datacopier_kill(dc);
-        dc->callback_pollhup(egc, dc, onwrite, -1);
-        return 1;
+    if (dc->callback_pollhup && (*revents & POLLHUP)) {
+        if (dc->callback_pollhup(egc, dc, onwrite)) {
+            LOG(DEBUG, "received POLLHUP on %s during copy of %s",
+                onwrite ? dc->writewhat : dc->readwhat, dc->copywhat);
+            libxl__datacopier_kill(dc);
+            return true;
+        } else {
+            LOG(DEBUG, "Ignoring POLLHUP on %s during copy of %s",
+                onwrite ? dc->writewhat : dc->readwhat, dc->copywhat);
+            *revents &= ~POLLHUP;
+            return false;
+        }
     }
-    return 0;
+
+    return false;
 }
 
 static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
@@ -204,7 +214,7 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
     libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, toread);
     STATE_AO_GC(dc->ao);
 
-    if (datacopier_pollhup_handled(egc, dc, revents, 0))
+    if (datacopier_handle_pollhup(egc, dc, &revents, 0))
         return;
 
     if (revents & ~POLLIN) {
@@ -279,7 +289,7 @@ static void datacopier_writable(libxl__egc *egc, libxl__ev_fd *ev,
     libxl__datacopier_state *dc = CONTAINER_OF(ev, *dc, towrite);
     STATE_AO_GC(dc->ao);
 
-    if (datacopier_pollhup_handled(egc, dc, revents, 1))
+    if (datacopier_handle_pollhup(egc, dc, &revents, 1))
         return;
 
     if (revents & ~POLLOUT) {
diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
index 1503101..db5ee02 100644
--- a/tools/libxl/libxl_bootloader.c
+++ b/tools/libxl/libxl_bootloader.c
@@ -31,8 +31,12 @@
 static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op);
 static void bootloader_keystrokes_copyfail(libxl__egc *egc,
        libxl__datacopier_state *dc, int onwrite, int errnoval);
+static bool bootloader_keystrokes_pollhup(libxl__egc *egc,
+       libxl__datacopier_state *dc, int onwrite);
 static void bootloader_display_copyfail(libxl__egc *egc,
        libxl__datacopier_state *dc, int onwrite, int errnoval);
+static bool bootloader_display_pollhup(libxl__egc *egc,
+       libxl__datacopier_state *dc, int onwrite);
 static void bootloader_domaindeath(libxl__egc*, libxl__domaindeathcheck *dc);
 static void bootloader_finished(libxl__egc *egc, libxl__ev_child *child,
                                 pid_t pid, int status);
@@ -520,7 +524,7 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
     bl->keystrokes.copywhat =
         GCSPRINTF("bootloader input for domain %"PRIu32, bl->domid);
     bl->keystrokes.callback =         bootloader_keystrokes_copyfail;
-    bl->keystrokes.callback_pollhup = bootloader_keystrokes_copyfail;
+    bl->keystrokes.callback_pollhup = bootloader_keystrokes_pollhup;
         /* pollhup gets called with errnoval==-1 which is not otherwise
          * possible since errnos are nonnegative, so it's unambiguous */
     rc = libxl__datacopier_start(&bl->keystrokes);
@@ -532,7 +536,7 @@ static void bootloader_gotptys(libxl__egc *egc, libxl__openpty_state *op)
     bl->display.copywhat =
         GCSPRINTF("bootloader output for domain %"PRIu32, bl->domid);
     bl->display.callback =         bootloader_display_copyfail;
-    bl->display.callback_pollhup = bootloader_display_copyfail;
+    bl->display.callback_pollhup = bootloader_display_pollhup;
     rc = libxl__datacopier_start(&bl->display);
     if (rc) goto out;
 
@@ -603,12 +607,26 @@ static void bootloader_keystrokes_copyfail(libxl__egc *egc,
     libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
     bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, errnoval);
 }
+static bool bootloader_keystrokes_pollhup(libxl__egc *egc,
+       libxl__datacopier_state *dc, int onwrite)
+{
+    libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
+    bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, -1);
+    return true;
+}
 static void bootloader_display_copyfail(libxl__egc *egc,
        libxl__datacopier_state *dc, int onwrite, int errnoval)
 {
     libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, display);
     bootloader_copyfail(egc, "bootloader output", bl, 1, onwrite, errnoval);
 }
+static bool bootloader_display_pollhup(libxl__egc *egc,
+       libxl__datacopier_state *dc, int onwrite)
+{
+    libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, display);
+    bootloader_copyfail(egc, "bootloader output", bl, 1, onwrite, -1);
+    return true;
+}
 
 static void bootloader_domaindeath(libxl__egc *egc, libxl__domaindeathcheck *dc)
 {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c2e7aa4..0c7c821 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2528,11 +2528,17 @@ _hidden void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
  *                 written when in read mode
  *     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.
- * In all cases copier is killed before calling this callback */
+ *
+ * If we get POLLHUP, we call callback_pollhup(..., onwrite) which may choose
+ * whether the POLLHUP is fatal with its return value.  If
+ * callback_pollhup==NULL this is an internal failure, as above.  The copier
+ * has never been killed before a pollhup callback, but will subequently be
+ * killed if the callback return true.
+ */
 typedef void libxl__datacopier_callback(libxl__egc *egc,
      libxl__datacopier_state *dc, int onwrite, int errnoval);
+typedef bool libxl__datacopier_pollhup_callback(libxl__egc *egc,
+     libxl__datacopier_state *dc, int onwrite);
 
 struct libxl__datacopier_buf {
     /* private to datacopier */
@@ -2550,7 +2556,7 @@ struct libxl__datacopier_state {
     const char *copywhat, *readwhat, *writewhat; /* for error msgs */
     FILE *log; /* gets a copy of everything */
     libxl__datacopier_callback *callback;
-    libxl__datacopier_callback *callback_pollhup;
+    libxl__datacopier_pollhup_callback *callback_pollhup;
     /* remaining fields are private to datacopier */
     libxl__ev_fd toread, towrite;
     ssize_t used;
-- 
1.7.10.4

Re: [PATCH 1/6] tools/libxl: Introduce min and max macros

[Ian Campbell]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> This is the same set used by libxc.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 934465a..a2b6fb7 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -108,6 +108,22 @@
>  
>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>  
> +#define min(X, Y) ({                             \
> +            const typeof (X) _x = (X);           \
> +            const typeof (Y) _y = (Y);           \
> +            (void) (&_x == &_y);                 \
> +            (_x < _y) ? _x : _y; })
> +#define max(X, Y) ({                             \
> +            const typeof (X) _x = (X);           \
> +            const typeof (Y) _y = (Y);           \
> +            (void) (&_x == &_y);                 \
> +            (_x > _y) ? _x : _y; })
> +
> +#define min_t(type,x,y) \
> +        ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
> +#define max_t(type,x,y) \
> +        ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
> +
>  #define LIBXL__LOGGING_ENABLED
>  
>  #ifdef LIBXL__LOGGING_ENABLED

Re: [PATCH 2/6] tools/libxl: Update datacopier to support sending data only

[Ian Campbell]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> datacopier is to read some data and write it out. If we
> have some data to send it over network, we cannot use
> datacopier. Update it to support this case.

Please can you clarify this commit message. Questions I'm left with
after reading it:

      * What is the relevance of "send it over network" here, why does
        it matter what the output fd is? Or is this something to do with
        the lack of an input fd (in which case where does the incoming
        data come from?)
      * Why can datacopier not currently be used in this case, what
        actually goes wrong?
      * What is the nature of the update which makes it work? (possibly
        becomes obvious after the previous answers)

Thanks.

Ian.

> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_aoutils.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index b10d2e1..3e0c0ae 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -309,9 +309,11 @@ int libxl__datacopier_start(libxl__datacopier_state *dc)
>  
>      libxl__datacopier_init(dc);
>  
> -    rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
> -                               dc->readfd, POLLIN);
> -    if (rc) goto out;
> +    if (dc->readfd >= 0) {
> +        rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
> +                                   dc->readfd, POLLIN);
> +        if (rc) goto out;
> +    }
>  
>      rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
>                                 dc->writefd, POLLOUT);

Re: [PATCH 3/6] tools/libxl: Allow adding larger amounts of prefixdata to datacopier

[Ian Campbell]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Previously, adding more than 1000 bytes of data would cause a segfault.
> Now, the maximum amount of data that can be added is limited by maxsz.

http://lists.xen.org/archives/html/xen-devel/2014-09/msg01806.html:
        struct libxl__datacopier_buf contains a fixed size 1000 byte
        statically allocated buffer so adding > 1000 bytes of data would
        cause it to overrun the buffer and overwrite other memory.
http://lists.xen.org/archives/html/xen-devel/2014-09/msg01813.html
        Yes, this should be the main point of the commit log though.

The commit log should mention that the current code overruns a static
1000 byte buffer and fixes it by allocating and chaining as many buffers
as are required for the amount of data.

> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_aoutils.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 3e0c0ae..6882ca3 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -160,6 +160,8 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>  {
>      EGC_GC;
>      libxl__datacopier_buf *buf;
> +    const uint8_t *ptr;
> +
>      /*
>       * It is safe for this to be called immediately after _start, as
>       * is documented in the public comment.  _start's caller must have
> @@ -170,12 +172,14 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>  
>      assert(len < dc->maxsz - dc->used);
>  
> -    buf = libxl__zalloc(NOGC, sizeof(*buf));
> -    buf->used = len;
> -    memcpy(buf->buf, data, len);
> +    for (ptr = data; len; len -= buf->used, ptr += buf->used) {
> +        buf = libxl__malloc(NOGC, sizeof(*buf));
> +        buf->used = min(len, sizeof(buf->buf));
> +        memcpy(buf->buf, ptr, buf->used);
>  
> -    dc->used += len;
> -    LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> +        dc->used += buf->used;
> +        LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> +    }
>  }
>  
>  static int datacopier_pollhup_handled(libxl__egc *egc,

Re: [PATCH 4/6] tools/libxl: Allow limiting amount copied by datacopier

[Ian Campbell]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Add a parameter, maxread, to limit the amount of data read from the
> source fd of a datacopier.

http://lists.xen.org/archives/html/xen-devel/2014-09/msg01802.html and
followups.

Re: [PATCH 5/6] tools/libxl: Extend datacopier to support reading into a buffer

[Ian Campbell]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Implement a read-only mode for libxl__datacopier.  The mode is invoked
> when readbuf is set and writefd is < 0.  On success, the callback passes
> the number of bytes read.

http://lists.xen.org/archives/html/xen-devel/2014-09/msg01803.html

> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_aoutils.c  |   58 +++++++++++++++++++++++++-----------------
>  tools/libxl/libxl_internal.h |    4 ++-
>  2 files changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 037244e..a402e5c 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -134,7 +134,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
>      STATE_AO_GC(dc->ao);
>      int rc;
>      
> -    if (dc->used) {
> +    if (dc->used && !dc->readbuf) {
>          if (!libxl__ev_fd_isregistered(&dc->towrite)) {
>              rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
>                                         dc->writefd, POLLOUT);
> @@ -147,7 +147,7 @@ static void datacopier_check_state(libxl__egc *egc, libxl__datacopier_state *dc)
>          }
>      } else if (!libxl__ev_fd_isregistered(&dc->toread) || dc->maxread == 0) {
>          /* we have had eof */
> -        datacopier_callback(egc, dc, 0, 0);
> +        datacopier_callback(egc, dc, 0, dc->readbuf ? dc->used : 0);
>          return;
>      } else {
>          /* nothing buffered, but still reading */
> @@ -215,24 +215,30 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>      }
>      assert(revents & POLLIN);
>      for (;;) {
> -        while (dc->used >= dc->maxsz) {
> -            libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
> -            dc->used -= rm->used;
> -            assert(dc->used >= 0);
> -            LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
> -            free(rm);
> -        }
> +        libxl__datacopier_buf *buf = NULL;
> +        int r;
> +
> +        if (dc->readbuf) {
> +            r = read(ev->fd, dc->readbuf + dc->used, dc->maxread);
> +        } else {
> +            while (dc->used >= dc->maxsz) {
> +                libxl__datacopier_buf *rm = LIBXL_TAILQ_FIRST(&dc->bufs);
> +                dc->used -= rm->used;
> +                assert(dc->used >= 0);
> +                LIBXL_TAILQ_REMOVE(&dc->bufs, rm, entry);
> +                free(rm);
> +            }
>  
> -        libxl__datacopier_buf *buf =
> -            LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
> -        if (!buf || buf->used >= sizeof(buf->buf)) {
> -            buf = malloc(sizeof(*buf));
> -            if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
> -            buf->used = 0;
> -            LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> -        }
> -        int r = read(ev->fd, buf->buf + buf->used,
> +            buf = LIBXL_TAILQ_LAST(&dc->bufs, libxl__datacopier_bufs);
> +            if (!buf || buf->used >= sizeof(buf->buf)) {
> +                buf = malloc(sizeof(*buf));
> +                if (!buf) libxl__alloc_failed(CTX, __func__, 1, sizeof(*buf));
> +                buf->used = 0;
> +                LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> +            }
> +            r = read(ev->fd, buf->buf + buf->used,
>                       min_t(size_t, sizeof(buf->buf) - buf->used, dc->maxread));
> +        }
>          if (r < 0) {
>              if (errno == EINTR) continue;
>              if (errno == EWOULDBLOCK) break;
> @@ -255,10 +261,12 @@ static void datacopier_readable(libxl__egc *egc, libxl__ev_fd *ev,
>                  return;
>              }
>          }
> -        buf->used += r;
> +        if (!dc->readbuf) {
> +            buf->used += r;
> +            assert(buf->used <= sizeof(buf->buf));
> +        }
>          dc->used += r;
>          dc->maxread -= r;
> -        assert(buf->used <= sizeof(buf->buf));
>          assert(dc->maxread >= 0);
>          if (dc->maxread == 0)
>              break;
> @@ -316,15 +324,19 @@ int libxl__datacopier_start(libxl__datacopier_state *dc)
>  
>      libxl__datacopier_init(dc);
>  
> +    assert(dc->readfd >= 0 || dc->writefd >= 0);
> +
>      if (dc->readfd >= 0) {
>          rc = libxl__ev_fd_register(gc, &dc->toread, datacopier_readable,
>                                     dc->readfd, POLLIN);
>          if (rc) goto out;
>      }
>  
> -    rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
> -                               dc->writefd, POLLOUT);
> -    if (rc) goto out;
> +    if (dc->writefd >= 0) {
> +        rc = libxl__ev_fd_register(gc, &dc->towrite, datacopier_writable,
> +                                   dc->writefd, POLLOUT);
> +        if (rc) goto out;
> +    }
>  
>      return 0;
>  
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index b7fd13d..c2e7aa4 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2524,7 +2524,8 @@ _hidden void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>  
>  /* onwrite==1 means failure happened when writing, logged, errnoval is valid
>   * onwrite==0 means failure happened when reading
> - *     errnoval==0 means we got eof and all data was written
> + *     errnoval>=0 means we got eof and all data was written or number of bytes
> + *                 written when in read mode
>   *     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);
> @@ -2553,6 +2554,7 @@ struct libxl__datacopier_state {
>      /* remaining fields are private to datacopier */
>      libxl__ev_fd toread, towrite;
>      ssize_t used;
> +    void *readbuf;
>      LIBXL_TAILQ_HEAD(libxl__datacopier_bufs, libxl__datacopier_buf) bufs;
>  };
>  

Re: [PATCH 1/6] tools/libxl: Introduce min and max macros

[Frediano Ziglio]

2015-02-18 16:34 GMT+00:00 Andrew Cooper <andrew.cooper3@citrix.com>:
> This is the same set used by libxc.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 934465a..a2b6fb7 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -108,6 +108,22 @@
>
>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>
> +#define min(X, Y) ({                             \
> +            const typeof (X) _x = (X);           \
> +            const typeof (Y) _y = (Y);           \
> +            (void) (&_x == &_y);                 \
> +            (_x < _y) ? _x : _y; })
> +#define max(X, Y) ({                             \
> +            const typeof (X) _x = (X);           \
> +            const typeof (Y) _y = (Y);           \
> +            (void) (&_x == &_y);                 \
> +            (_x > _y) ? _x : _y; })
> +
> +#define min_t(type,x,y) \
> +        ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
> +#define max_t(type,x,y) \
> +        ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
> +

Surely nobody will complain about these defines but according to
standard (ie https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier)
identifiers that start with double underscore are reserved. Here you
used _x, _y and __x, __y as it's a single patch make at least
coherent.

>  #define LIBXL__LOGGING_ENABLED
>
>  #ifdef LIBXL__LOGGING_ENABLED
> --
> 1.7.10.4
>

Frediano

Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

[Ian Campbell]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> POLLIN|POLLHUP is a valid revent to recieve from poll() when the far end has
> hung up, but data is still available to be read.
> 
> Currently all POLLHUPs are fatal.  This is a problem when using the legacy
> stream conversion script which will exit 0 when it has completed its task and
> has left valid data in the pipe.  In this case, libxl wishes to read until
> eof, rather than dying because the conversion script exited.
> 
> Adjustments are as follows:
>  1. The datacopier pollhup callback changes type to include a return value.
>  2. datacopier_handle_pollhup() takes revents by pointer, and calls the
>     datacopier pollhup callback ahead of killing the datacopier.
>  3. The callback may now indicate that the POLLHUP is not fatal, in which case
>     datacopier_handle_pollhup() mask POLLHUP out of revents, and
>     datacopier_{read,write}able() allowed to proceed as before.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_aoutils.c    |   34 ++++++++++++++++++++++------------
>  tools/libxl/libxl_bootloader.c |   22 ++++++++++++++++++++--
>  tools/libxl/libxl_internal.h   |   14 ++++++++++----
>  3 files changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index a402e5c..f3e5f98 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -182,21 +182,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>      }
>  }
>  
> -static int datacopier_pollhup_handled(libxl__egc *egc,
> +/*
> + * Handle a POLLHUP signal on a datacopier fd.  Return boolean indicating
> + * whether further processing should cease.

Should it return true to cease? Or false to indicate not to continue?

(More importantly than here this should be specified precisely in the
doc header near the callback definition, which it isn't, it just says
"with its return value").
> @@ -603,12 +607,26 @@ static void bootloader_keystrokes_copyfail(libxl__egc *egc,
>      libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
>      bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, errnoval);
>  }
> +static bool bootloader_keystrokes_pollhup(libxl__egc *egc,
> +       libxl__datacopier_state *dc, int onwrite)
> +{
> +    libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
> +    bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, -1);

Is there no valid errno here?

It's a bit of a shame that callers which don't care about specific
pollhup handling have to provide two practically identical handlers.

Is there any mileage in suggesting that the default callback type used
for copyfail too might return a bool too in order that they can be
shared? Even if it must always return True.

Or perhaps some method to indicate that on pollhup, if pollhip callback
is NULL, use the regular callback? (where some method might be the
pollhup_callback==NULL itself?)

> + *
> + * If we get POLLHUP, we call callback_pollhup(..., onwrite) which may choose
> + * whether the POLLHUP is fatal with its return value.

Please precisely specific the return value meanings here.

>   If
> + * callback_pollhup==NULL this is an internal failure, as above.  The copier
> + * has never been killed before a pollhup callback, but will subequently be

"subsequently"

> + * killed if the callback return true.
> + */
>  typedef void libxl__datacopier_callback(libxl__egc *egc,
>       libxl__datacopier_state *dc, int onwrite, int errnoval);
> +typedef bool libxl__datacopier_pollhup_callback(libxl__egc *egc,
> +     libxl__datacopier_state *dc, int onwrite);
>  
>  struct libxl__datacopier_buf {
>      /* private to datacopier */
> @@ -2550,7 +2556,7 @@ struct libxl__datacopier_state {
>      const char *copywhat, *readwhat, *writewhat; /* for error msgs */
>      FILE *log; /* gets a copy of everything */
>      libxl__datacopier_callback *callback;
> -    libxl__datacopier_callback *callback_pollhup;
> +    libxl__datacopier_pollhup_callback *callback_pollhup;
>      /* remaining fields are private to datacopier */
>      libxl__ev_fd toread, towrite;
>      ssize_t used;

Re: [PATCH 1/6] tools/libxl: Introduce min and max macros

[Andrew Cooper]

On 20/02/15 10:42, Frediano Ziglio wrote:
> 2015-02-18 16:34 GMT+00:00 Andrew Cooper <andrew.cooper3@citrix.com>:
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 934465a..a2b6fb7 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -108,6 +108,22 @@
>>
>>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>>
>> +#define min(X, Y) ({                             \
>> +            const typeof (X) _x = (X);           \
>> +            const typeof (Y) _y = (Y);           \
>> +            (void) (&_x == &_y);                 \
>> +            (_x < _y) ? _x : _y; })
>> +#define max(X, Y) ({                             \
>> +            const typeof (X) _x = (X);           \
>> +            const typeof (Y) _y = (Y);           \
>> +            (void) (&_x == &_y);                 \
>> +            (_x > _y) ? _x : _y; })
>> +
>> +#define min_t(type,x,y) \
>> +        ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
>> +#define max_t(type,x,y) \
>> +        ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
>> +
> Surely nobody will complain about these defines but according to
> standard (ie https://www.securecoding.cert.org/confluence/display/seccode/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier)
> identifiers that start with double underscore are reserved. Here you
> used _x, _y and __x, __y as it's a single patch make at least
> coherent.

I will adjust in v2.  No point breaking the rules given that it is
trivial to fix.

~Andrew

Re: [PATCH 2/6] tools/libxl: Update datacopier to support sending data only

[Andrew Cooper]

On 20/02/15 10:27, Ian Campbell wrote:
> On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> datacopier is to read some data and write it out. If we
>> have some data to send it over network, we cannot use
>> datacopier. Update it to support this case.
> Please can you clarify this commit message. Questions I'm left with
> after reading it:
>
>       * What is the relevance of "send it over network" here, why does
>         it matter what the output fd is? Or is this something to do with
>         the lack of an input fd (in which case where does the incoming
>         data come from?)
>       * Why can datacopier not currently be used in this case, what
>         actually goes wrong?
>       * What is the nature of the update which makes it work? (possibly
>         becomes obvious after the previous answers)
>
> Thanks.

This is one of several patches which didn't get included as part of the
libxl remus series in the end (i.e. git rebase didn't drop this hunk
out), but is still needed for migration v2.

Currently, the datacopier only functions when copying from readfd to
writefd until EOF is hit on readfd.

The datacopier infrastructure already has prefixdata which will be
inserted into writefd ahead of the content of readfd, but lacks the
ability to simply copy from a local buffer to writefd.

This patch makes the lack of readfd non-fatal, which allows the rest of
the existing infrastructure function as a proper copy from local buffer.

~Andrew

Re: [PATCH 2/6] tools/libxl: Update datacopier to support sending data only

[Ian Campbell]

On Fri, 2015-02-20 at 11:10 +0000, Andrew Cooper wrote:
> On 20/02/15 10:27, Ian Campbell wrote:
> > On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> >> From: Wen Congyang <wency@cn.fujitsu.com>
> >>
> >> datacopier is to read some data and write it out. If we
> >> have some data to send it over network, we cannot use
> >> datacopier. Update it to support this case.
> > Please can you clarify this commit message. Questions I'm left with
> > after reading it:
> >
> >       * What is the relevance of "send it over network" here, why does
> >         it matter what the output fd is? Or is this something to do with
> >         the lack of an input fd (in which case where does the incoming
> >         data come from?)
> >       * Why can datacopier not currently be used in this case, what
> >         actually goes wrong?
> >       * What is the nature of the update which makes it work? (possibly
> >         becomes obvious after the previous answers)
> >
> > Thanks.
> 
> This is one of several patches which didn't get included as part of the
> libxl remus series in the end (i.e. git rebase didn't drop this hunk
> out), but is still needed for migration v2.
> 
> Currently, the datacopier only functions when copying from readfd to
> writefd until EOF is hit on readfd.
> 
> The datacopier infrastructure already has prefixdata which will be
> inserted into writefd ahead of the content of readfd, but lacks the
> ability to simply copy from a local buffer to writefd.
> 
> This patch makes the lack of readfd non-fatal, which allows the rest of
> the existing infrastructure function as a proper copy from local buffer.

Where "copy from local buffer" is using the prefixdata support?

Anyway, please expand the commit message with all that info.

> 
> ~Andrew
> 

Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

[Andrew Cooper]

On 20/02/15 10:43, Ian Campbell wrote:
> On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
>> POLLIN|POLLHUP is a valid revent to recieve from poll() when the far end has
>> hung up, but data is still available to be read.
>>
>> Currently all POLLHUPs are fatal.  This is a problem when using the legacy
>> stream conversion script which will exit 0 when it has completed its task and
>> has left valid data in the pipe.  In this case, libxl wishes to read until
>> eof, rather than dying because the conversion script exited.
>>
>> Adjustments are as follows:
>>  1. The datacopier pollhup callback changes type to include a return value.
>>  2. datacopier_handle_pollhup() takes revents by pointer, and calls the
>>     datacopier pollhup callback ahead of killing the datacopier.
>>  3. The callback may now indicate that the POLLHUP is not fatal, in which case
>>     datacopier_handle_pollhup() mask POLLHUP out of revents, and
>>     datacopier_{read,write}able() allowed to proceed as before.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxl/libxl_aoutils.c    |   34 ++++++++++++++++++++++------------
>>  tools/libxl/libxl_bootloader.c |   22 ++++++++++++++++++++--
>>  tools/libxl/libxl_internal.h   |   14 ++++++++++----
>>  3 files changed, 52 insertions(+), 18 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
>> index a402e5c..f3e5f98 100644
>> --- a/tools/libxl/libxl_aoutils.c
>> +++ b/tools/libxl/libxl_aoutils.c
>> @@ -182,21 +182,31 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>>      }
>>  }
>>  
>> -static int datacopier_pollhup_handled(libxl__egc *egc,
>> +/*
>> + * Handle a POLLHUP signal on a datacopier fd.  Return boolean indicating
>> + * whether further processing should cease.
> Should it return true to cease? Or false to indicate not to continue?
>
> (More importantly than here this should be specified precisely in the
> doc header near the callback definition, which it isn't, it just says
> "with its return value").

The sense as stated is correct, but I will spell it out.

>> @@ -603,12 +607,26 @@ static void bootloader_keystrokes_copyfail(libxl__egc *egc,
>>      libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
>>      bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, errnoval);
>>  }
>> +static bool bootloader_keystrokes_pollhup(libxl__egc *egc,
>> +       libxl__datacopier_state *dc, int onwrite)
>> +{
>> +    libxl__bootloader_state *bl = CONTAINER_OF(dc, *bl, keystrokes);
>> +    bootloader_copyfail(egc, "bootloader input", bl, 0, onwrite, -1);
> Is there no valid errno here?

-1 is documented way of indicating a POLLHUP.

>
> It's a bit of a shame that callers which don't care about specific
> pollhup handling have to provide two practically identical handlers.

Up until this patch, all users either provided no POLLHUP handler, or
provided the same handler for both function pointers.

>
> Is there any mileage in suggesting that the default callback type used
> for copyfail too might return a bool too in order that they can be
> shared? Even if it must always return True.
>
> Or perhaps some method to indicate that on pollhup, if pollhip callback
> is NULL, use the regular callback? (where some method might be the
> pollhup_callback==NULL itself?)

Previously, every callback was issued after the datacopier had already
been killed.  Now, the pollhup callback is called before the kill has
occurred, and is able to prevent the kill from happening.

A different and far less invasive approach might be to have a per-fd
revent ignore mask.  This would at least allow the callbacks to be made
when the datacopier is in a consistent state.

~Andrew

Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

[Ian Campbell]

On Fri, 2015-02-20 at 13:55 +0000, Andrew Cooper wrote:

> >
> > It's a bit of a shame that callers which don't care about specific
> > pollhup handling have to provide two practically identical handlers.
> 
> Up until this patch, all users either provided no POLLHUP handler, or
> provided the same handler for both function pointers.

Yes, and I'm saying it is a shame that those in the latter class now
have to provide two callbacks instead of the one they used before.

> > Is there any mileage in suggesting that the default callback type used
> > for copyfail too might return a bool too in order that they can be
> > shared? Even if it must always return True.
> >
> > Or perhaps some method to indicate that on pollhup, if pollhip callback
> > is NULL, use the regular callback? (where some method might be the
> > pollhup_callback==NULL itself?)
> 
> Previously, every callback was issued after the datacopier had already
> been killed.  Now, the pollhup callback is called before the kill has
> occurred, and is able to prevent the kill from happening.
> 
> A different and far less invasive approach might be to have a per-fd
> revent ignore mask.  This would at least allow the callbacks to be made
> when the datacopier is in a consistent state.

Ian, any thoughts on this?

Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal"):
> On Fri, 2015-02-20 at 13:55 +0000, Andrew Cooper wrote:
> > A different and far less invasive approach might be to have a per-fd
> > revent ignore mask.  This would at least allow the callbacks to be made
> > when the datacopier is in a consistent state.
> 
> Ian, any thoughts on this?

I think there is a real bug but the patch is misconceived :-/.

Andrew says on IRC:

18:02 <andyhhp> Diziet: POLLHUP|POLLIN is a valid revent to recieve
18:03 <andyhhp> which happens when the writer has written into the
                pipe and closed the fd

This is true [1] and will indeed cause the datacopier to quit before
it has finished copying all the data, which I think is indeed a bug.

On the other hand, some fd objects can return POLLHUP without other
indications.  In those cases we should fail.

I think the right answer is for the dc to ignore POLLHUP iff there are
other bits set which are going to be handled.

Care needs to be taken about how this interacts with
  7253e0fd1aeb3ae7d4714bcc1d86b846b3331995
  libxl: react correctly to bootloader pty master POLLHUP

Ian.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html

Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal

[Andrew Cooper]

On 03/03/15 18:10, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 6/6] tools/libxl: Fix datacopier POLLHUP handling to not always be fatal"):
>> On Fri, 2015-02-20 at 13:55 +0000, Andrew Cooper wrote:
>>> A different and far less invasive approach might be to have a per-fd
>>> revent ignore mask.  This would at least allow the callbacks to be made
>>> when the datacopier is in a consistent state.
>> Ian, any thoughts on this?
> I think there is a real bug but the patch is misconceived :-/.
>
> Andrew says on IRC:
>
> 18:02 <andyhhp> Diziet: POLLHUP|POLLIN is a valid revent to recieve
> 18:03 <andyhhp> which happens when the writer has written into the
>                 pipe and closed the fd
>
> This is true [1] and will indeed cause the datacopier to quit before
> it has finished copying all the data, which I think is indeed a bug.
>
> On the other hand, some fd objects can return POLLHUP without other
> indications.  In those cases we should fail.
>
> I think the right answer is for the dc to ignore POLLHUP iff there are
> other bits set which are going to be handled.
>
> Care needs to be taken about how this interacts with
>   7253e0fd1aeb3ae7d4714bcc1d86b846b3331995
>   libxl: react correctly to bootloader pty master POLLHUP
>
> Ian.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html

I don't think anything good can come from ignoring a POLLHUP on
write_fd, as a subsequent call to write() is either going to hit a hard
error, or block.  (Indeed, [1] indicates that POLLHUP and POLLOUT are
mutually exclusive.)

On the read side, transforming POLLHUP|POLLIN to POLLIN should be safe,
as read() will continue to work until EOF.

Given [1], this should be safe even interacting with 7253e0fd1 as
POLLHUP should be set for every subsequent poll(), even once EOF has
been reached.

~Andrew