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

[Ian Campbell <ian.campbell@citrix.com>]

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.