[Patch v2 0/3] Some bugfix patches

[Patch v2 0/3] Some bugfix patches

[Wen Congyang]

These bugs are found when we implement COLO, or rebase
COLO to upstream xen. They are independent patches, so
post them in separate series.

The codes are also hosted on github:
https://github.com/wencongyang/xen/tree/bugfix-s1-v2

Wen Congyang (3):
  check if mfn is supported by IOCTL_PRIVCMD_MMAPBATCH before calling
    ioctl()
  correct xc_domain_save()'s return value
  update libxl__device_disk_from_xs_be() to support blktap device

 tools/libxc/xc_domain_save.c |  59 ++++++++++++-----------
 tools/libxc/xc_linux_osdep.c |  34 ++++++++++++++
 tools/libxl/libxl.c          | 109 +++++++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_blktap2.c  |  12 ++++-
 4 files changed, 168 insertions(+), 46 deletions(-)

-- 
1.9.3

[Patch v2 1/3] check if mfn is supported by IOCTL_PRIVCMD_MMAPBATCH before calling ioctl()

[Wen Congyang]

If mfn is valid but is large enough to have the top nibble (the
"error nibble") set then there is no way to do proper error
reporting, because the mfn and the error code will get intermixed.
So we should check the input mfn before calling ioctl(). If the
user doesn't want a mapping at a given index, he can use ~0UL.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxc/xc_linux_osdep.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c
index a19e4b6..816e616 100644
--- a/tools/libxc/xc_linux_osdep.c
+++ b/tools/libxc/xc_linux_osdep.c
@@ -156,6 +156,28 @@ static int xc_map_foreign_batch_single(int fd, uint32_t dom,
     return rc;
 }
 
+/*
+ * If the mfn is valid but is large enough to have the top nibble (the
+ * "error nibble") set then there is no way to do proper error reporting,
+ * because the mfn and the error code will get intermixed
+ */
+static int check_mfn_for_mmapbatch(const xen_pfn_t *arr, int num)
+{
+    int i;
+
+    for ( i = 0; i < num; i++ )
+    {
+        /* the caller indicates that they don't want a mapping at index i */
+        if ( arr[i] == ~0UL )
+            continue;
+
+        if ( arr[i] & PRIVCMD_MMAPBATCH_MFN_ERROR )
+            return 1;
+    }
+
+    return 0;
+}
+
 static void *linux_privcmd_map_foreign_batch(xc_interface *xch, xc_osdep_handle h,
                                              uint32_t dom, int prot,
                                              xen_pfn_t *arr, int num)
@@ -307,6 +329,12 @@ static void *linux_privcmd_map_foreign_bulk(xc_interface *xch, xc_osdep_handle h
         xen_pfn_t *pfn;
         unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), XC_PAGE_SHIFT);
 
+        if ( check_mfn_for_mmapbatch(arr, num) )
+        {
+            (void)munmap(addr, (unsigned long)num << XC_PAGE_SHIFT);
+            return NULL;
+        }
+
         if ( pfn_arr_size <= XC_PAGE_SIZE )
             pfn = alloca(num * sizeof(*pfn));
         else
@@ -333,6 +361,12 @@ static void *linux_privcmd_map_foreign_bulk(xc_interface *xch, xc_osdep_handle h
 
         for ( i = 0; i < num; ++i )
         {
+            if ( arr[i] == ~0UL )
+            {
+                err[i] = -EINVAL;
+                continue;
+            }
+
             switch ( pfn[i] ^ arr[i] )
             {
             case 0:
-- 
1.9.3

[Patch v2 2/3] correct xc_domain_save()'s return value

[Wen Congyang]

If suspend_and_state() fails, the errno may be 0. But we assume
that the errno is not 0. So remove assert().

If the callback checkpoint() fails, it means that remus
failed. But xc_domain_save() returns 0.

This patch changes xc_domain_save()'s behavior, and the errno is
undefined even if xc_domain_save() returns 1.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/libxc/xc_domain_save.c | 59 +++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 254fdb3..d96fd24 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -807,7 +807,7 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     xc_dominfo_t info;
     DECLARE_DOMCTL;
 
-    int rc, frc, i, j, last_iter = 0, iter = 0;
+    int rc = 1, frc, i, j, last_iter = 0, iter = 0;
     int live  = (flags & XCFLAGS_LIVE);
     int debug = (flags & XCFLAGS_DEBUG);
     int superpages = !!hvm;
@@ -2073,8 +2073,12 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     goto out_rc;
 
  out:
-    rc = errno;
-    assert(rc);
+    /*
+     * When we come here, some erroe happened. But the errno may be undefined.
+     * For example, the callback suspend() fails, and we cannot get correct
+     * errno, because it may be implemented in libxl.
+     */
+    rc = 1;
  out_rc:
     completed = 1;
 
@@ -2113,30 +2117,34 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     compressing = (flags & XCFLAGS_CHECKPOINT_COMPRESS);
 
     /* checkpoint_cb can spend arbitrarily long in between rounds */
-    if (!rc && callbacks->checkpoint &&
-        callbacks->checkpoint(callbacks->data) > 0)
+    if ( !rc && callbacks->checkpoint )
     {
-        /* reset stats timer */
-        print_stats(xch, dom, 0, &time_stats, &shadow_stats, 0);
-
-        /* last_iter = 1; */
-        if ( suspend_and_state(callbacks->suspend, callbacks->data, xch,
-                               io_fd, dom, &info) )
+        if ( callbacks->checkpoint(callbacks->data) > 0 )
         {
-            ERROR("Domain appears not to have suspended");
-            goto out;
-        }
-        DPRINTF("SUSPEND shinfo %08lx\n", info.shared_info_frame);
-        print_stats(xch, dom, 0, &time_stats, &shadow_stats, 1);
+            /* reset stats timer */
+            print_stats(xch, dom, 0, &time_stats, &shadow_stats, 0);
 
-        if ( xc_shadow_control(xch, dom,
-                               XEN_DOMCTL_SHADOW_OP_CLEAN, HYPERCALL_BUFFER(to_send),
-                               dinfo->p2m_size, NULL, 0, &shadow_stats) != dinfo->p2m_size )
-        {
-            PERROR("Error flushing shadow PT");
-        }
+            /* last_iter = 1; */
+            if ( suspend_and_state(callbacks->suspend, callbacks->data, xch,
+                                   io_fd, dom, &info) )
+            {
+                ERROR("Domain appears not to have suspended");
+                goto out;
+            }
+            DPRINTF("SUSPEND shinfo %08lx\n", info.shared_info_frame);
+            print_stats(xch, dom, 0, &time_stats, &shadow_stats, 1);
 
-        goto copypages;
+            if ( xc_shadow_control(xch, dom,
+                                   XEN_DOMCTL_SHADOW_OP_CLEAN, HYPERCALL_BUFFER(to_send),
+                                   dinfo->p2m_size, NULL, 0, &shadow_stats) != dinfo->p2m_size )
+            {
+                PERROR("Error flushing shadow PT");
+            }
+
+            goto copypages;
+        }
+        else
+            rc = 1;
     }
 
     if ( tmem_saved != 0 && live )
@@ -2174,11 +2182,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
     free(hvm_buf);
     outbuf_free(&ob_pagebuf);
 
-    errno = rc;
 exit:
-    DPRINTF("Save exit of domid %u with errno=%d\n", dom, errno);
+    DPRINTF("Save exit of domid %u with rc=%d\n", dom, rc);
 
-    return !!errno;
+    return rc;
 }
 
 /*
-- 
1.9.3

Re: [Patch v2 2/3] correct xc_domain_save()'s return value

[Ian Jackson]

Wen Congyang writes ("[Patch v2 2/3] correct xc_domain_save()'s return value"):
> If suspend_and_state() fails, the errno may be 0. But we assume
> that the errno is not 0. So remove assert().

Thanks for spotting this.

I think this is going in the wrong direction.  Perhaps we could
instead do something like the patch below ?  Please let me know what
you think.

If you think this is a better idea, please submit it as a proper patch
with a proper commit message.

(Ideally we would fix the actual suspend hook in libxl, to always set
errno, but that's too invasive a set of changes to do now, I think.)

Thanks,
Ian.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 40bbac8..3ab9dd8 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -35,7 +35,9 @@
 /* callbacks provided by xc_domain_save */
 struct save_callbacks {
     /* Called after expiration of checkpoint interval,
-     * to suspend the guest.
+     * to suspend the guest.  Returns 1 for success, or 0 for failure.
+     * On failure it should ideally set errno.  (If it leaves errno
+     * as 0, EIO will be used instead.)
      */
     int (*suspend)(void* data);
 
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index 254fdb3..444aac6 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -361,9 +361,15 @@ static int suspend_and_state(int (*suspend)(void*), void* data,
                              xc_interface *xch, int io_fd, int dom,
                              xc_dominfo_t *info)
 {
+    errno = 0;
     if ( !(*suspend)(data) )
     {
-        ERROR("Suspend request failed");
+        if (!errno) {
+            errno = EIO;
+            ERROR("Suspend request failed (without errno, using EINVAL)");
+        } else {
+            ERROR("Suspend request failed");
+        }
         return -1;
     }

Re: [Patch v2 2/3] correct xc_domain_save()'s return value

[Ian Campbell]

On Fri, 2014-11-14 at 14:57 +0000, Ian Jackson wrote:
> +            errno = EIO;
> +            ERROR("Suspend request failed (without errno, using EINVAL)");

You mean EIO in the error message, I think.

Ian.

Re: [Patch v2 2/3] correct xc_domain_save()'s return value

[Ian Jackson]

Ian Campbell writes ("Re: [Patch v2 2/3] correct xc_domain_save()'s return value"):
> On Fri, 2014-11-14 at 14:57 +0000, Ian Jackson wrote:
> > +            errno = EIO;
> > +            ERROR("Suspend request failed (without errno, using EINVAL)");
> 
> You mean EIO in the error message, I think.

Oops, yes.

Ian

Re: [Patch v2 2/3] correct xc_domain_save()'s return value

[Wen Congyang]

On 11/14/2014 10:57 PM, Ian Jackson wrote:
> Wen Congyang writes ("[Patch v2 2/3] correct xc_domain_save()'s return value"):
>> If suspend_and_state() fails, the errno may be 0. But we assume
>> that the errno is not 0. So remove assert().
> 
> Thanks for spotting this.
> 
> I think this is going in the wrong direction.  Perhaps we could
> instead do something like the patch below ?  Please let me know what
> you think.
> 
> If you think this is a better idea, please submit it as a proper patch
> with a proper commit message.

OK, I will do it.

> 
> (Ideally we would fix the actual suspend hook in libxl, to always set
> errno, but that's too invasive a set of changes to do now, I think.)

libxl and helper program are two programs, and we should update the interface
between libxl and the hepler program first. We can do it later.

Thanks
Wen Congyang

> 
> Thanks,
> Ian.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
> index 40bbac8..3ab9dd8 100644
> --- a/tools/libxc/include/xenguest.h
> +++ b/tools/libxc/include/xenguest.h
> @@ -35,7 +35,9 @@
>  /* callbacks provided by xc_domain_save */
>  struct save_callbacks {
>      /* Called after expiration of checkpoint interval,
> -     * to suspend the guest.
> +     * to suspend the guest.  Returns 1 for success, or 0 for failure.
> +     * On failure it should ideally set errno.  (If it leaves errno
> +     * as 0, EIO will be used instead.)
>       */
>      int (*suspend)(void* data);
>  
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index 254fdb3..444aac6 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -361,9 +361,15 @@ static int suspend_and_state(int (*suspend)(void*), void* data,
>                               xc_interface *xch, int io_fd, int dom,
>                               xc_dominfo_t *info)
>  {
> +    errno = 0;
>      if ( !(*suspend)(data) )
>      {
> -        ERROR("Suspend request failed");
> +        if (!errno) {
> +            errno = EIO;
> +            ERROR("Suspend request failed (without errno, using EINVAL)");
> +        } else {
> +            ERROR("Suspend request failed");
> +        }
>          return -1;
>      }
>  
> .
> 

[Patch v2 3/3] update libxl__device_disk_from_xs_be() to support blktap device

[Wen Congyang]

The API libxl_device_disk_list() will return all disks. But the type is wrong
if it is a tapdisk device, because we get the type from the params xenstore
node, not the tapdisk-params xenstore node. Update libxl__device_disk_from_xs_be(),
try tapdisk-params xenstore node first to support blktap device.

But even if we get type from tapdisk-params xenstore node, the type is still wrong,
because we store wrong format into tapdisk-params xenstore node: if the format is
raw, we store aio into either the tapdisk-params xenstore node. And we cannot use
the API libxl_disk_format_from_string() to get the format from the string. So use
libxl_disk_format_to_string() to instead of libxl__device_disk_string_of_format()
to store the format to tapdisk-params xenstore node.

Also update libxl__device_destroy_tapdisk() due to tapdisk-params change.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl.c         | 109 ++++++++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_blktap2.c |  12 ++++-
 2 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9c72df2..efc3ca6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2404,7 +2404,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                 }
                 flexarray_append(back, "tapdisk-params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
-                    libxl__device_disk_string_of_format(disk->format),
+                    libxl_disk_format_to_string(disk->format),
                     disk->pdev_path));
 
                 /* tap backends with scripts are rejected by
@@ -2514,6 +2514,55 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
     device_disk_add(egc, domid, disk, aodev, NULL, NULL);
 }
 
+/*
+ * read params from xenstore node. The params's format should be
+ * "format:path" or "path". If the xenstore node doesn't exist,
+ * return 0, and both *format and *path are NULL. On success,
+ * the caller should be free *path by hand.
+ */
+static int read_params(libxl__gc *gc, const char *be_path,
+                       char **format, char **path)
+{
+    char *params, *tmp;
+
+    assert(format != NULL && path != NULL);
+
+    *format = NULL;
+    *path = NULL;
+
+    params = libxl__xs_read(gc, XBT_NULL, be_path);
+    if (!params) {
+        if (errno == ENOENT)
+            return 0;
+
+        LOGE(ERROR, "cannot read %s", be_path);
+        return ERROR_FAIL;
+    }
+
+    tmp = strchr(params, ':');
+    if (!tmp) {
+        *path = strdup(params);
+        if (*path == NULL)
+            goto out;
+
+        return 0;
+    }
+
+    *path = strdup(tmp + 1);
+    if (*path == NULL)
+        goto out;
+
+    *tmp = '\0';
+    *format = params;
+
+    return 0;
+
+out:
+    LOGE(ERROR, "no memory to store path");
+
+    return ERROR_NOMEM;
+}
+
 static int libxl__device_disk_from_xs_be(libxl__gc *gc,
                                          const char *be_path,
                                          libxl_device_disk *disk)
@@ -2531,24 +2580,48 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
         goto cleanup;
     }
 
-    /* "params" may not be present; but everything else must be. */
-    tmp = xs_read(ctx->xsh, XBT_NULL,
-                  libxl__sprintf(gc, "%s/params", be_path), &len);
-    if (tmp && strchr(tmp, ':')) {
-        disk->pdev_path = strdup(strchr(tmp, ':') + 1);
-        free(tmp);
-    } else {
-        disk->pdev_path = tmp;
-    }
-
-
-    tmp = libxl__xs_read(gc, XBT_NULL,
-                         libxl__sprintf(gc, "%s/type", be_path));
-    if (!tmp) {
-        LOG(ERROR, "Missing xenstore node %s/type", be_path);
+    /* "tapdisk-params" is only for tapdisk */
+    rc = read_params(gc, GCSPRINTF("%s/tapdisk-params", be_path),
+                     &tmp, &disk->pdev_path);
+    if (rc)
         goto cleanup;
+    if (tmp || disk->pdev_path) {
+        if (!tmp) {
+            LOG(ERROR, "corrupted tapdisk-params: %s", disk->pdev_path);
+            goto cleanup;
+        }
+        rc = libxl_disk_format_from_string(tmp, &disk->format);
+        if (rc) {
+            LOG(ERROR, "unknown disk format: %s", tmp);
+            goto cleanup;
+        }
+        if (disk->format != LIBXL_DISK_FORMAT_VHD &&
+            disk->format != LIBXL_DISK_FORMAT_RAW) {
+            /* We can get here only when the xenstore node is corrupted */
+            LOG(ERROR, "unsupported tapdisk format: %s", tmp);
+            goto cleanup;
+        }
+
+        /*
+         * The backend is tapdisk, so we store tapdev in params, and
+         * phy in type(see device_disk_add())
+         */
+        disk->backend = LIBXL_DISK_BACKEND_TAP;
+    } else {
+        /* "params" may not be present; but everything else must be. */
+        rc = read_params(gc, GCSPRINTF("%s/params", be_path),
+                         &tmp, &disk->pdev_path);
+        if (rc)
+            goto cleanup;
+
+        tmp = libxl__xs_read(gc, XBT_NULL,
+                             libxl__sprintf(gc, "%s/type", be_path));
+        if (!tmp) {
+            LOG(ERROR, "Missing xenstore node %s/type", be_path);
+            goto cleanup;
+        }
+        libxl_string_to_backend(ctx, tmp, &(disk->backend));
     }
-    libxl_string_to_backend(ctx, tmp, &(disk->backend));
 
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
                          libxl__sprintf(gc, "%s/dev", be_path), &len);
@@ -2583,8 +2656,6 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
     }
     disk->is_cdrom = !strcmp(tmp, "cdrom");
 
-    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
-
     return 0;
 cleanup:
     libxl_device_disk_dispose(disk);
diff --git a/tools/libxl/libxl_blktap2.c b/tools/libxl/libxl_blktap2.c
index 2053403..7656fe4 100644
--- a/tools/libxl/libxl_blktap2.c
+++ b/tools/libxl/libxl_blktap2.c
@@ -54,8 +54,9 @@ char *libxl__blktap_devpath(libxl__gc *gc,
 int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
 {
     char *type, *disk;
-    int err;
+    int err, rc;
     tap_list_t tap;
+    libxl_disk_format format;
 
     type = libxl__strdup(gc, params);
 
@@ -67,6 +68,15 @@ int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
 
     *disk++ = '\0';
 
+    /* type may be raw */
+    rc = libxl_disk_format_from_string(type, &format);
+    if (rc < 0) {
+        LOG(ERROR, "invalid disk type %s", type);
+        return rc;
+    }
+
+    type = libxl__device_disk_string_of_format(format);
+
     err = tap_ctl_find(type, disk, &tap);
     if (err < 0) {
         /* returns -errno */
-- 
1.9.3

Re: [Patch v2 3/3] update libxl__device_disk_from_xs_be() to support blktap device

[Ian Jackson]

Wen Congyang writes ("[Patch v2 3/3] update libxl__device_disk_from_xs_be() to support blktap device"):
> [stuff]

Thanks for your attention to the details of this rather neglected
area.

This patch seems related to 
  [RFC Patch v4 8/9] store correct format into tapdisk-params/params
from Mon, 22 Sep 2014 13:59:20 +0800.

Is that right ?  I confess I don't know exactly how all of the tapdisk
plumbing works, but I do expect it to be rather ... idiosyncratic.  Is
this patch (and the other one) for blktap1 or blktap2 ?

Is there is some kind of document I should be looking at explaining
how the relevant blktap's xenstore protocol works ?  I found
tools/blktap/README and tools/blktap2/README but they don't seem to
answer the question.

I think it will be difficult to review this patch without a clear
description of the intended design.  You seem to have done much of the
reverse-engineering necessary, so perhaps you could provide a sketch
of the relevant information with your patch - perhaps as a doc comment
somewhere in the code, provided in a pre-patch ?  Also, I would like
to understand more clearly how this functionality interacts with Wei
Liu's new approach to libxl domain configuration management.

I think I should avoid reviewing the concrete implementation until I
feel I understand what the patch ought to do.  Otherwise we risk
iterating to improve details when the overall approach isn't
necessarily right.

Thanks,
Ian.

Re: [Patch v2 0/3] Some bugfix patches

[Ian Jackson]

Wen Congyang writes ("[Patch v2 0/3] Some bugfix patches"):
> These bugs are found when we implement COLO, or rebase
> COLO to upstream xen. They are independent patches, so
> post them in separate series.

Thanks.  I can't seem to see any discussion of these bugfixes.  They
seem to have been dropped.  (I myself came across them while gardening
my inbox.)

IMO they should be considered for 4.5.  But they need review by the
relevant maintainers first.  I think the first patch need attention
from the x86 maintainers.  I will look at the other two.

Ian.