[PATCH] libxl: refactor toolstack save restore code

[PATCH] libxl: refactor toolstack save restore code

[Wei Liu]

This patch does following things:
1. Document v1 format.
2. Factor out function to handle QEMU restore data and function to
   handle v1 blob for restore path.
3. Refactor save function to generate different blobs in the order
   specified in format specification.
4. Change functions to use "goto out" idiom.

No functional changes introduced.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
I wrote this patch when exploring the idea of have toolstack save / restore v2
to record max pages information. Though that idea has been abandon it wouldn't
hurt to have clearer code structure and documentation.
---
 tools/libxl/libxl_dom.c | 247 +++++++++++++++++++++++++++++-------------------
 1 file changed, 150 insertions(+), 97 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 867172a..23c4691 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1032,6 +1032,15 @@ struct libxl__physmap_info {
     char name[];
 };
 
+/* Bump version every time when toolstack saved data changes.
+ * Different types of data are arranged in the specified order.
+ *
+ * Version 1:
+ *   uint32_t version
+ *   QEMU physmap data:
+ *     uint32_t count
+ *     libxl__physmap_info * count
+ */
 #define TOOLSTACK_SAVE_VERSION 1
 
 static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
@@ -1043,66 +1052,97 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
                                        phys_offset, node);
 }
 
-int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
-                             uint32_t size, void *user)
+static int libxl__toolstack_restore_qemu(libxl__gc *gc, uint32_t domid,
+                                         const uint8_t *buf, uint32_t size)
 {
-    libxl__save_helper_state *shs = user;
-    libxl__domain_create_state *dcs = CONTAINER_OF(shs, *dcs, shs);
-    STATE_AO_GC(dcs->ao);
-    int i, ret;
-    const uint8_t *ptr = buf;
-    uint32_t count = 0, version = 0;
-    struct libxl__physmap_info* pi;
+    int rc, i;
+    uint32_t count;
     char *xs_path;
     uint32_t dm_domid;
+    struct libxl__physmap_info *pi;
 
-    LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size);
-
-    if (size < sizeof(version) + sizeof(count)) {
+    if (size < sizeof(count)) {
         LOG(ERROR, "wrong size");
-        return -1;
-    }
-
-    memcpy(&version, ptr, sizeof(version));
-    ptr += sizeof(version);
-
-    if (version != TOOLSTACK_SAVE_VERSION) {
-        LOG(ERROR, "wrong version");
-        return -1;
+        rc = -1;
+        goto out;
     }
 
-    memcpy(&count, ptr, sizeof(count));
-    ptr += sizeof(count);
+    memcpy(&count, buf, sizeof(count));
+    buf += sizeof(count);
 
-    if (size < sizeof(version) + sizeof(count) +
-            count * (sizeof(struct libxl__physmap_info))) {
+    if (size < sizeof(count) + count*(sizeof(struct libxl__physmap_info))) {
         LOG(ERROR, "wrong size");
-        return -1;
+        rc = -1;
+        goto out;
     }
 
     dm_domid = libxl_get_stubdom_id(CTX, domid);
     for (i = 0; i < count; i++) {
-        pi = (struct libxl__physmap_info*) ptr;
-        ptr += sizeof(struct libxl__physmap_info) + pi->namelen;
+        pi = (struct libxl__physmap_info*) buf;
+        buf += sizeof(struct libxl__physmap_info) + pi->namelen;
 
         xs_path = restore_helper(gc, dm_domid, domid,
                                  pi->phys_offset, "start_addr");
-        ret = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->start_addr);
-        if (ret)
-            return -1;
+        rc = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->start_addr);
+        if (rc) goto out;
+
         xs_path = restore_helper(gc, dm_domid, domid, pi->phys_offset, "size");
-        ret = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->size);
-        if (ret)
-            return -1;
+        rc = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->size);
+        if (rc) goto out;
+
         if (pi->namelen > 0) {
             xs_path = restore_helper(gc, dm_domid, domid,
                                      pi->phys_offset, "name");
-            ret = libxl__xs_write(gc, 0, xs_path, "%s", pi->name);
-            if (ret)
-                return -1;
+            rc = libxl__xs_write(gc, 0, xs_path, "%s", pi->name);
+            if (rc) goto out;
         }
     }
-    return 0;
+
+    rc = 0;
+out:
+    return rc;
+
+}
+
+static int libxl__toolstack_restore_v1(libxl__gc *gc, uint32_t domid,
+                                       const uint8_t *buf, uint32_t size)
+{
+    return libxl__toolstack_restore_qemu(gc, domid, buf, size);
+}
+
+int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
+                             uint32_t size, void *user)
+{
+    libxl__save_helper_state *shs = user;
+    libxl__domain_create_state *dcs = CONTAINER_OF(shs, *dcs, shs);
+    STATE_AO_GC(dcs->ao);
+    int rc;
+    const uint8_t *ptr = buf;
+    uint32_t version = 0, bufsize;
+
+    LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size);
+
+    if (size < sizeof(version)) {
+        LOG(ERROR, "wrong size");
+        rc = -1;
+        goto out;
+    }
+
+    memcpy(&version, ptr, sizeof(version));
+    ptr += sizeof(version);
+    bufsize = size - sizeof(version);
+
+    switch (version) {
+    case 1:
+        rc = libxl__toolstack_restore_v1(gc, domid, ptr, bufsize);
+        break;
+    default:
+        LOG(ERROR, "wrong version");
+        rc = -1;
+    }
+
+out:
+    return rc;
 }
 
 /*==================== Domain suspend (save) ====================*/
@@ -1685,81 +1725,94 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
         uint32_t *len, void *dss_void)
 {
     libxl__domain_suspend_state *dss = dss_void;
+    int rc;
     STATE_AO_GC(dss->ao);
     int i = 0;
-    char *start_addr = NULL, *size = NULL, *phys_offset = NULL, *name = NULL;
-    unsigned int num = 0;
-    uint32_t count = 0, version = TOOLSTACK_SAVE_VERSION, namelen = 0;
+    uint32_t version = TOOLSTACK_SAVE_VERSION;
     uint8_t *ptr = NULL;
-    char **entries = NULL;
-    struct libxl__physmap_info *pi;
-    uint32_t dm_domid;
-
-    dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-    entries = libxl__xs_directory(gc, 0,
-                libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"),
-                &num);
-    count = num;
+    rc = -1;
 
-    *len = sizeof(version) + sizeof(count);
+    /* Version number */
+    *len = sizeof(version);
     *buf = calloc(1, *len);
+    if (*buf == NULL) goto out;
     ptr = *buf;
-    if (*buf == NULL)
-        return -1;
-
     memcpy(ptr, &version, sizeof(version));
-    ptr += sizeof(version);
-    memcpy(ptr, &count, sizeof(count));
-    ptr += sizeof(count);
 
-    for (i = 0; i < count; i++) {
-        unsigned long offset;
-        char *xs_path;
-        phys_offset = entries[i];
-        if (phys_offset == NULL) {
-            LOG(ERROR, "phys_offset %d is NULL", i);
-            return -1;
-        }
+    /* QEMU physmap data */
+    {
+        char **entries = NULL, *xs_path;
+        struct libxl__physmap_info *pi;
+        uint32_t dm_domid;
+        char *start_addr = NULL, *size = NULL, *phys_offset = NULL;
+        char *name = NULL;
+        unsigned int num = 0;
+        uint32_t count = 0, namelen = 0;
+
+        dm_domid = libxl_get_stubdom_id(CTX, domid);
+
+        xs_path = libxl__device_model_xs_path(gc, dm_domid, domid,
+                                              "/physmap");
+        entries = libxl__xs_directory(gc, 0, xs_path, &num);
+        count = num;
+
+        *len += sizeof(count);
+        *buf = realloc(*buf, *len);
+        if (*buf == NULL) goto out;
+        ptr = *buf + sizeof(version);
+        memcpy(ptr, &count, sizeof(count));
+        ptr += sizeof(count);
+
+        for (i = 0; i < count; i++) {
+            unsigned long offset;
+            phys_offset = entries[i];
+            if (phys_offset == NULL) {
+                LOG(ERROR, "phys_offset %d is NULL", i);
+                goto out;
+            }
 
-        xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "start_addr");
-        start_addr = libxl__xs_read(gc, 0, xs_path);
-        if (start_addr == NULL) {
-            LOG(ERROR, "%s is NULL", xs_path);
-            return -1;
-        }
+            xs_path = physmap_path(gc, dm_domid, domid, phys_offset,
+                                   "start_addr");
+            start_addr = libxl__xs_read(gc, 0, xs_path);
+            if (start_addr == NULL) {
+                LOG(ERROR, "%s is NULL", xs_path);
+                goto out;
+            }
 
-        xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "size");
-        size = libxl__xs_read(gc, 0, xs_path);
-        if (size == NULL) {
-            LOG(ERROR, "%s is NULL", xs_path);
-            return -1;
-        }
+            xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "size");
+            size = libxl__xs_read(gc, 0, xs_path);
+            if (size == NULL) {
+                LOG(ERROR, "%s is NULL", xs_path);
+                goto out;
+            }
 
-        xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "name");
-        name = libxl__xs_read(gc, 0, xs_path);
-        if (name == NULL)
-            namelen = 0;
-        else
-            namelen = strlen(name) + 1;
-        *len += namelen + sizeof(struct libxl__physmap_info);
-        offset = ptr - (*buf);
-        *buf = realloc(*buf, *len);
-        if (*buf == NULL)
-            return -1;
-        ptr = (*buf) + offset;
-        pi = (struct libxl__physmap_info *) ptr;
-        pi->phys_offset = strtoll(phys_offset, NULL, 16);
-        pi->start_addr = strtoll(start_addr, NULL, 16);
-        pi->size = strtoll(size, NULL, 16);
-        pi->namelen = namelen;
-        memcpy(pi->name, name, namelen);
-        ptr += sizeof(struct libxl__physmap_info) + namelen;
+            xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "name");
+            name = libxl__xs_read(gc, 0, xs_path);
+            if (name == NULL)
+                namelen = 0;
+            else
+                namelen = strlen(name) + 1;
+            *len += namelen + sizeof(struct libxl__physmap_info);
+            offset = ptr - (*buf);
+            *buf = realloc(*buf, *len);
+            if (*buf == NULL) goto out;
+            ptr = (*buf) + offset;
+            pi = (struct libxl__physmap_info *) ptr;
+            pi->phys_offset = strtoll(phys_offset, NULL, 16);
+            pi->start_addr = strtoll(start_addr, NULL, 16);
+            pi->size = strtoll(size, NULL, 16);
+            pi->namelen = namelen;
+            memcpy(pi->name, name, namelen);
+            ptr += sizeof(struct libxl__physmap_info) + namelen;
+        }
     }
 
     LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, *len);
 
-    return 0;
+    rc = 0;
+out:
+    return rc;
 }
 
 static void libxl__domain_suspend_callback(void *data)
-- 
1.9.1

Re: [PATCH] libxl: refactor toolstack save restore code

[Andrew Cooper]

On 05/06/15 18:01, Wei Liu wrote:
> This patch does following things:
> 1. Document v1 format.
> 2. Factor out function to handle QEMU restore data and function to
>    handle v1 blob for restore path.
> 3. Refactor save function to generate different blobs in the order
>    specified in format specification.
> 4. Change functions to use "goto out" idiom.
>
> No functional changes introduced.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> I wrote this patch when exploring the idea of have toolstack save / restore v2
> to record max pages information. Though that idea has been abandon it wouldn't
> hurt to have clearer code structure and documentation.
> ---
>  tools/libxl/libxl_dom.c | 247 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 150 insertions(+), 97 deletions(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 867172a..23c4691 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1032,6 +1032,15 @@ struct libxl__physmap_info {
>      char name[];
>  };
>  
> +/* Bump version every time when toolstack saved data changes.
> + * Different types of data are arranged in the specified order.
> + *
> + * Version 1:
> + *   uint32_t version
> + *   QEMU physmap data:
> + *     uint32_t count
> + *     libxl__physmap_info * count
> + */

Ah fantastic - this was some information which IanJ asked me to detail
in the libxl v2 stream spec, and I had not gotten around to working out
yet.  (Current handling was just to pass it verbatim back to libxl.)

It is probably the kind of thing which needs splitting into appropriate
v2 records, rather than having a structured binary blob inside a
structered stream, both defined for the same library.

OOI, why does libxl need to send this information for Qemu? We don't
have any equivalent in XenServer.

~Andrew

Re: [PATCH] libxl: refactor toolstack save restore code

[Wei Liu]

On Fri, Jun 05, 2015 at 06:46:35PM +0100, Andrew Cooper wrote:
> On 05/06/15 18:01, Wei Liu wrote:
> > This patch does following things:
> > 1. Document v1 format.
> > 2. Factor out function to handle QEMU restore data and function to
> >    handle v1 blob for restore path.
> > 3. Refactor save function to generate different blobs in the order
> >    specified in format specification.
> > 4. Change functions to use "goto out" idiom.
> >
> > No functional changes introduced.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > I wrote this patch when exploring the idea of have toolstack save / restore v2
> > to record max pages information. Though that idea has been abandon it wouldn't
> > hurt to have clearer code structure and documentation.
> > ---
> >  tools/libxl/libxl_dom.c | 247 +++++++++++++++++++++++++++++-------------------
> >  1 file changed, 150 insertions(+), 97 deletions(-)
> >
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 867172a..23c4691 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -1032,6 +1032,15 @@ struct libxl__physmap_info {
> >      char name[];
> >  };
> >  
> > +/* Bump version every time when toolstack saved data changes.
> > + * Different types of data are arranged in the specified order.
> > + *
> > + * Version 1:
> > + *   uint32_t version
> > + *   QEMU physmap data:
> > + *     uint32_t count
> > + *     libxl__physmap_info * count
> > + */
> 
> Ah fantastic - this was some information which IanJ asked me to detail
> in the libxl v2 stream spec, and I had not gotten around to working out
> yet.  (Current handling was just to pass it verbatim back to libxl.)
> 
> It is probably the kind of thing which needs splitting into appropriate
> v2 records, rather than having a structured binary blob inside a
> structered stream, both defined for the same library.
> 
> OOI, why does libxl need to send this information for Qemu? We don't
> have any equivalent in XenServer.
> 

It is for upstream QEMU to save / restore physical map information.  I
think XenServer has not yet switched to upstream QEMU so you don't have
this.

I don't know the history, Stefano may have better idea.

Wei.

> ~Andrew

Re: [PATCH] libxl: refactor toolstack save restore code

[George Dunlap]

On Fri, Jun 5, 2015 at 7:15 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Fri, Jun 05, 2015 at 06:46:35PM +0100, Andrew Cooper wrote:
>> On 05/06/15 18:01, Wei Liu wrote:
>> > This patch does following things:
>> > 1. Document v1 format.
>> > 2. Factor out function to handle QEMU restore data and function to
>> >    handle v1 blob for restore path.
>> > 3. Refactor save function to generate different blobs in the order
>> >    specified in format specification.
>> > 4. Change functions to use "goto out" idiom.
>> >
>> > No functional changes introduced.
>> >
>> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> > ---
>> > I wrote this patch when exploring the idea of have toolstack save / restore v2
>> > to record max pages information. Though that idea has been abandon it wouldn't
>> > hurt to have clearer code structure and documentation.
>> > ---
>> >  tools/libxl/libxl_dom.c | 247 +++++++++++++++++++++++++++++-------------------
>> >  1 file changed, 150 insertions(+), 97 deletions(-)
>> >
>> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> > index 867172a..23c4691 100644
>> > --- a/tools/libxl/libxl_dom.c
>> > +++ b/tools/libxl/libxl_dom.c
>> > @@ -1032,6 +1032,15 @@ struct libxl__physmap_info {
>> >      char name[];
>> >  };
>> >
>> > +/* Bump version every time when toolstack saved data changes.
>> > + * Different types of data are arranged in the specified order.
>> > + *
>> > + * Version 1:
>> > + *   uint32_t version
>> > + *   QEMU physmap data:
>> > + *     uint32_t count
>> > + *     libxl__physmap_info * count
>> > + */
>>
>> Ah fantastic - this was some information which IanJ asked me to detail
>> in the libxl v2 stream spec, and I had not gotten around to working out
>> yet.  (Current handling was just to pass it verbatim back to libxl.)
>>
>> It is probably the kind of thing which needs splitting into appropriate
>> v2 records, rather than having a structured binary blob inside a
>> structered stream, both defined for the same library.
>>
>> OOI, why does libxl need to send this information for Qemu? We don't
>> have any equivalent in XenServer.
>>
>
> It is for upstream QEMU to save / restore physical map information.  I
> think XenServer has not yet switched to upstream QEMU so you don't have
> this.

I'm not super familiar with the qemu stuff, but it certainly seems to
be the case that qemu-trad does *not* expect to know the physical
layout of memory, whereas qemu-upstream *does*.  This is the source of
the MMIO hole resizing issues as well -- on qemu-trad, moving the
memory around didn't cause any problems; on qemu-upstream, if you add
memory to a guest's physmap where it wasn't before, if qemu doesn't
know about it, it will crash when trying to access it.  I assume
that's what this section is for.

 -George

Re: [PATCH] libxl: refactor toolstack save restore code

[Ian Campbell]

On Fri, 2015-06-05 at 18:01 +0100, Wei Liu wrote:
> This patch does following things:
> 1. Document v1 format.
> 2. Factor out function to handle QEMU restore data and function to
>    handle v1 blob for restore path.
> 3. Refactor save function to generate different blobs in the order
>    specified in format specification.
> 4. Change functions to use "goto out" idiom.
> 
> No functional changes introduced.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> I wrote this patch when exploring the idea of have toolstack save / restore v2
> to record max pages information. Though that idea has been abandon it wouldn't
> hurt to have clearer code structure and documentation.
> ---
>  tools/libxl/libxl_dom.c | 247 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 150 insertions(+), 97 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 867172a..23c4691 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1032,6 +1032,15 @@ struct libxl__physmap_info {
>      char name[];
>  };
>  
> +/* Bump version every time when toolstack saved data changes.
> + * Different types of data are arranged in the specified order.
> + *
> + * Version 1:
> + *   uint32_t version
> + *   QEMU physmap data:
> + *     uint32_t count
> + *     libxl__physmap_info * count
> + */
>  #define TOOLSTACK_SAVE_VERSION 1
>  
>  static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
> @@ -1043,66 +1052,97 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
>                                         phys_offset, node);
>  }
>  
> -int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
> -                             uint32_t size, void *user)
> +static int libxl__toolstack_restore_qemu(libxl__gc *gc, uint32_t domid,
> +                                         const uint8_t *buf, uint32_t size)
>  {
> -    libxl__save_helper_state *shs = user;
> -    libxl__domain_create_state *dcs = CONTAINER_OF(shs, *dcs, shs);
> -    STATE_AO_GC(dcs->ao);
> -    int i, ret;
> -    const uint8_t *ptr = buf;
> -    uint32_t count = 0, version = 0;
> -    struct libxl__physmap_info* pi;
> +    int rc, i;
> +    uint32_t count;
>      char *xs_path;
>      uint32_t dm_domid;
> +    struct libxl__physmap_info *pi;
>  
> -    LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size);
> -
> -    if (size < sizeof(version) + sizeof(count)) {
> +    if (size < sizeof(count)) {
>          LOG(ERROR, "wrong size");
> -        return -1;
> -    }
> -
> -    memcpy(&version, ptr, sizeof(version));
> -    ptr += sizeof(version);
> -
> -    if (version != TOOLSTACK_SAVE_VERSION) {
> -        LOG(ERROR, "wrong version");
> -        return -1;
> +        rc = -1;
> +        goto out;

Per-coding style a variable called rc must never contain anything other
than a libxl error code. This function even seems to use it for both,
which is even worse. It should either be called "r" or "ret" and be -1
style or it should be rc and use ERROR_FOO style, consistently on or the
other.

>      }
>  
> -    memcpy(&count, ptr, sizeof(count));
> -    ptr += sizeof(count);
> +    memcpy(&count, buf, sizeof(count));
> +    buf += sizeof(count);

Calling it ptr instead of buf would have made the patch a lot smaller
and easier to see the wood for the trees etc.


> +int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
> +                             uint32_t size, void *user)
> +{
> +    libxl__save_helper_state *shs = user;
> +    libxl__domain_create_state *dcs = CONTAINER_OF(shs, *dcs, shs);
> +    STATE_AO_GC(dcs->ao);
> +    int rc;
> +    const uint8_t *ptr = buf;
> +    uint32_t version = 0, bufsize;
> +
> +    LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size);
> +
> +    if (size < sizeof(version)) {
> +        LOG(ERROR, "wrong size");
> +        rc = -1;

As above.

Ian.