Re: [PATCH v6 COLO 03/15] primary vm suspend/get_dirty_pfn/resume/checkpoint code

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

On Mon, 2015-06-08 at 11:45 +0800, Yang Hongyang wrote:
> diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
> index 86bcf9c..d5902a6 100644
> --- a/tools/libxc/include/xenguest.h
> +++ b/tools/libxc/include/xenguest.h
> @@ -75,6 +75,18 @@ struct save_callbacks {
>       */
>      int (*toolstack_save)(uint32_t domid, uint8_t **buf, uint32_t *len, void *data);
>  
> +    /* Called after the guest is suspended.
> +     *
> +     * returns the list of dirty pfn:
> +     *  struct {
> +     *      uint64_t count;
> +     *      uint64_t pfn[];
> +     *  };

Seeing this comment and then a callback which returns a uint8_t* makes
me suspicious. Can we not do something a bit more typesafe here, like
returning a pointer to a suitable struct?

> +     *
> +     *  Note: the caller must free the return value.
> +     */
> +    uint8_t *(*get_dirty_pfn)(void *data);
> +
>      /* to be provided as the last argument to each callback function */
>      void* data;
>  };
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 10d3d82..1145ae4 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -17,6 +17,7 @@
>  #include "libxl_osdeps.h"
>  
>  #include "libxl_internal.h"
> +#include "libxl_colo.h"
>  
>  #define PAGE_TO_MEMKB(pages) ((pages) * 4)
>  #define BACKEND_STRING_SIZE 5
> @@ -841,7 +842,10 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>      assert(info);
>  
>      /* Point of no return */
> -    libxl__remus_setup(egc, &dss->rs);
> +    if (libxl_defbool_val(info->colo))

libxl code must arrange to have called libxl_defbool_setdefault before
using libxl_defbool_val, which I don't see here. There is a big block of
such settings near the top of this function which you should add to.

On the other hand -- is it possible for a caller to say they don't care
what kind of check pointing they want and have libxl decide? If not then
it doesn't make sense to use a defbool, a regular bool would be
appropriate.

I'm also wondering to what extent COLO could be considered an extension
to Remus, as opposed to an alternative -- iow I'm unsure if reusing
libxl_domain_remus_start as the API makes sense (the implementation
could still be shared where appropriate).

> diff --git a/tools/libxl/libxl_colo.h b/tools/libxl/libxl_colo.h
> index 91df275..26a2563 100644
> --- a/tools/libxl/libxl_colo.h
> +++ b/tools/libxl/libxl_colo.h
> @@ -35,4 +35,14 @@ extern void libxl__colo_restore_teardown(libxl__egc *egc,
>                                           libxl__colo_restore_state *crs,
>                                           int rc);
>  
> +extern void libxl__colo_save_domain_suspend_callback(void *data);
> +extern void libxl__colo_save_domain_resume_callback(void *data);
> +extern void libxl__colo_save_domain_checkpoint_callback(void *data);
> +extern void libxl__colo_save_get_dirty_pfn_callback(void *data);
> +extern void libxl__colo_save_setup(libxl__egc *egc,
> +                                   libxl__colo_save_state *css);
> +extern void libxl__colo_save_teardown(libxl__egc *egc,
> +                                      libxl__colo_save_state *css,
> +                                      int rc);

Should all be marked _hidden I think?

[...]