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

[PATCH v2 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.

Changes in v2:
 * Rewrite most commit message (but no code change)
 * Rework the POLLHUP fix from scratch

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

---
v2: Don't use reserved identifiers in min_t/max_t
---
 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..fcbec7f 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)                                               \
+    ({ const type _x = (x); const type _y = (y); _x < _y ? _x: _y; })
+#define max_t(type, x, y)                                               \
+    ({ const type _x = (x); const type _y = (y); _x > _y ? _x: _y; })
+
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
-- 
1.7.10.4

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

[Andrew Cooper]

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

Currently, starting a datacopier requires a valid read and write fd, but this
is a problem when purely sending data from a local buffer to a writable fd.

The prefixdata mechanism already exists and works for inserting data from a
local buffer ahead of reading from the read fd.

Make the lack of a read fd non-fatal.  A datacopier with no read fd, but some
prefixdata will write the prefixdata to the write fd and complete successfully.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
[Rewrite commit message]
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>

---
v2: Rewrite commit message.  No code change
---
 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 v2 3/6] tools/libxl: Avoid overrunning static buffer with prefixdata

[Andrew Cooper]

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

An individual datacopier_buf contains a static buffer of 1000 bytes.
Attempting to add prefixdata of more than 1000 bytes would overrun the buffer
and cause heap corruption.

Instead, split the prefixdata and chain together multiple datacopier buffers.
This allows for an arbitrary quantity of prefixdata to be added to 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>

---
v2: Rewrite commit message.  No code change
---
 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 v2 4/6] tools/libxl: Allow limiting amount copied by datacopier

[Andrew Cooper]

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

Currently, a datacopier will unconditionally read until EOF on its read fd.

For migration v2, libxl needs to read records of a specific length out of the
migration stream, without reading any further data.

Introduce a parameter, maxread, which may be used to stop the datacopier ahead
of reaching EOF.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
[Rewrite commit message]
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>

---
v2: Rewrite commit message.  No code change
---
 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 a16d4a1..ffa36da 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 fcbec7f..22921c7 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 v2 5/6] tools/libxl: Extend datacopier to support reading into a buffer

[Andrew Cooper]

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

Currently a datacopier may source its data from an fd or local buffer, but its
destination must be an fd.  For migration v2, libxl needs to read from the
migration stream into a local buffer.

Implement a "read into local buffer" mode, invoked when readbuf is set and
writefd is -1.  On success, the callback passes the number of bytes read.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
[Rewrite commit message]
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>

---
v2: Rewrite commit message.  No code change
---
 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 22921c7..3e1d78a 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 v2 6/6] tools/libxl: Adjust datacopiers POLLHUP handling when the fd is also readable

[Andrew Cooper]

POLLHUP|POLLIN is a valid revent to receive when there is readable data in a
pipe, but the writable fd has been closed.  This occurs in migration v2 when
the legacy conversion process (which transforms the data inline) completes and
exits successfully.

In the case that there is data to read, suppress the POLLHUP.  POSIX states
that the hangup state is latched[1], which means it will reoccur on subsequent
poll() calls.  The datacopier is thus provided the opportunity to read until
EOF, if possible.

A POLLHUP on its own is treated exactly as before, indicating a different
error with the fd.

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

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>

---
v2: Rework solution from scratch
---
 tools/libxl/libxl_aoutils.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index a402e5c..a5ad3e5 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -204,6 +204,9 @@ 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 ((revents & (POLLHUP|POLLIN)) == (POLLHUP|POLLIN))
+        revents &= ~POLLHUP;
+
     if (datacopier_pollhup_handled(egc, dc, revents, 0))
         return;
 
-- 
1.7.10.4

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

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> 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>
> 
> ---
> v2: Don't use reserved identifiers in min_t/max_t
> ---
>  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..fcbec7f 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);           \

You've avoided the reserved-by-C names here, but IIRC a single
underscore is reserved by POSIX.

However since you've just copied from libxc, I think we can live with
this until someone find the energy to fix them all:
        Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

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

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> Currently, starting a datacopier requires a valid read and write fd, but this
> is a problem when purely sending data from a local buffer to a writable fd.
> 
> The prefixdata mechanism already exists and works for inserting data from a
> local buffer ahead of reading from the read fd.
> 
> Make the lack of a read fd non-fatal.  A datacopier with no read fd, but some
> prefixdata will write the prefixdata to the write fd and complete successfully.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> [Rewrite commit message]
> 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>

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

Re: [PATCH v2 3/6] tools/libxl: Avoid overrunning static buffer with prefixdata

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> An individual datacopier_buf contains a static buffer of 1000 bytes.
> Attempting to add prefixdata of more than 1000 bytes would overrun the buffer
> and cause heap corruption.
> 
> Instead, split the prefixdata and chain together multiple datacopier buffers.
> This allows for an arbitrary quantity of prefixdata to be added to 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>

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

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

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Currently, a datacopier will unconditionally read until EOF on its read fd.
> 
> For migration v2, libxl needs to read records of a specific length out of the
> migration stream, without reading any further data.
> 
> Introduce a parameter, maxread, which may be used to stop the datacopier ahead
> of reaching EOF.

maxread is ok as a parameter but not really in the dc.

It's unfortunate that "toread" is already used. Perhaps "remaining" (or
remread if you want to get terse)?

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

SSIZE_MAX exists for this I think.

But, doesn't this mean that if the bootloader receives INT_MAX (or
SIZE_MAX) keystrokes it will mysteriously stop working? Or do we already
have that via maxsz?

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

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Currently a datacopier may source its data from an fd or local buffer, but its
> destination must be an fd.  For migration v2, libxl needs to read from the
> migration stream into a local buffer.
> 
> Implement a "read into local buffer" mode, invoked when readbuf is set and
> writefd is -1.  On success, the callback passes the number of bytes read.

> +        if (dc->readbuf) {
> +            r = read(ev->fd, dc->readbuf + dc->used, dc->maxread);

Should the final parameter not be maxread - used?

I assume the caller is expected to make maxread and the readbuf size
consistent (which could BTW be mentioned in an update to the comments
relating to this interface).

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 22921c7..3e1d78a 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

Nit: the last should be errnoval < 0.

I'm not sure how I feel about using a field called errnoval to store
something other than an errno though. I suppose I can live with it.

>   * 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 v2 6/6] tools/libxl: Adjust datacopiers POLLHUP handling when the fd is also readable

[Ian Campbell]

On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> POLLHUP|POLLIN is a valid revent to receive when there is readable data in a
> pipe, but the writable fd has been closed.  This occurs in migration v2 when
> the legacy conversion process (which transforms the data inline) completes and
> exits successfully.
> 
> In the case that there is data to read, suppress the POLLHUP.  POSIX states
> that the hangup state is latched[1], which means it will reoccur on subsequent
> poll() calls.  The datacopier is thus provided the opportunity to read until
> EOF, if possible.
> 
> A POLLHUP on its own is treated exactly as before, indicating a different
> error with the fd.
> 
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html
> 
> 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>

Having looked at the previous thread I'm satisfied that this matches
what was discussed there, but I think the final ack needs to come from
Ian.

> 
> ---
> v2: Rework solution from scratch
> ---
>  tools/libxl/libxl_aoutils.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index a402e5c..a5ad3e5 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -204,6 +204,9 @@ 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 ((revents & (POLLHUP|POLLIN)) == (POLLHUP|POLLIN))
> +        revents &= ~POLLHUP;
> +
>      if (datacopier_pollhup_handled(egc, dc, revents, 0))
>          return;
>  

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

[Andrew Cooper]

On 11/03/15 12:15, Ian Campbell wrote:
> On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
>> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> Currently, a datacopier will unconditionally read until EOF on its read fd.
>>
>> For migration v2, libxl needs to read records of a specific length out of the
>> migration stream, without reading any further data.
>>
>> Introduce a parameter, maxread, which may be used to stop the datacopier ahead
>> of reaching EOF.
> maxread is ok as a parameter but not really in the dc.
>
> It's unfortunate that "toread" is already used. Perhaps "remaining" (or
> remread if you want to get terse)?

ssize_t bytes_to_read perhaps? with -1 being a sentinel for "until EOF" ?

~Andrew

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

[Ian Campbell]

On Wed, 2015-03-11 at 12:23 +0000, Andrew Cooper wrote:
> On 11/03/15 12:15, Ian Campbell wrote:
> > On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
> >> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> >>
> >> Currently, a datacopier will unconditionally read until EOF on its read fd.
> >>
> >> For migration v2, libxl needs to read records of a specific length out of the
> >> migration stream, without reading any further data.
> >>
> >> Introduce a parameter, maxread, which may be used to stop the datacopier ahead
> >> of reaching EOF.
> > maxread is ok as a parameter but not really in the dc.
> >
> > It's unfortunate that "toread" is already used. Perhaps "remaining" (or
> > remread if you want to get terse)?
> 
> ssize_t bytes_to_read perhaps? with -1 being a sentinel for "until EOF" ?

WFM.

[PATCH v2 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>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

---
v2: Don't use reserved identifiers in min_t/max_t
---
 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..fcbec7f 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)                                               \
+    ({ const type _x = (x); const type _y = (y); _x < _y ? _x: _y; })
+#define max_t(type, x, y)                                               \
+    ({ const type _x = (x); const type _y = (y); _x > _y ? _x: _y; })
+
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
-- 
1.7.10.4

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

[Andrew Cooper]

On 12/03/15 11:01, Andrew Cooper wrote:
> This is the same set used by libxc.
>

Please ignore this - I had an accident with git-send-email.

~Andrew

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

[Ross Lagerwall]

On 03/11/2015 12:19 PM, Ian Campbell wrote:
> On Fri, 2015-03-06 at 19:05 +0000, Andrew Cooper wrote:
>> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> Currently a datacopier may source its data from an fd or local buffer, but its
>> destination must be an fd.  For migration v2, libxl needs to read from the
>> migration stream into a local buffer.
>>
>> Implement a "read into local buffer" mode, invoked when readbuf is set and
>> writefd is -1.  On success, the callback passes the number of bytes read.
>
>> +        if (dc->readbuf) {
>> +            r = read(ev->fd, dc->readbuf + dc->used, dc->maxread);
>
> Should the final parameter not be maxread - used?

No, because maxread is decreased after every read. This makes more sense 
if it is renamed to bytes_to_read (as it is in V3).

>
> I assume the caller is expected to make maxread and the readbuf size
> consistent (which could BTW be mentioned in an update to the comments
> relating to this interface).

Yes, done in V3.

>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 22921c7..3e1d78a 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
>
> Nit: the last should be errnoval < 0.
>
> I'm not sure how I feel about using a field called errnoval to store
> something other than an errno though. I suppose I can live with it.
>
>>    * 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;
>>   };
>>
>
>


-- 
Ross Lagerwall