Re: [PATCH v11 14/27] secondary vm suspend/resume/checkpoint code

From: Ian Jackson <Ian.Jackson@eu.citrix.com>
To: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Cc: Lars Kurth <lars.kurth@citrix.com>, Wei Liu <wei.liu2@citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Wen Congyang <wency@cn.fujitsu.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Dong Eddie <eddie.dong@intel.com>,
	xen devel <xen-devel@lists.xen.org>,
	Anthony Perard <anthony.perard@citrix.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v11 14/27] secondary vm suspend/resume/checkpoint code
Date: Fri, 4 Mar 2016 17:11:33 +0000	[thread overview]
Message-ID: <22233.49605.789962.95800@mariner.uk.xensource.com> (raw)
In-Reply-To: <1457080891-26054-15-git-send-email-xiecl.fnst@cn.fujitsu.com>

Changlong Xie writes ("[PATCH v11 14/27] secondary vm suspend/resume/checkpoint code"):
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> Secondary vm is running in colo mode. So we will do
> the following things again and again:

I don't propose to review this in detail.  Skimreading it, it looks
plausible.  I don't think a detailed review is needed.

I will review the changes to the core code.

> diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
> index 5d9f497..2bfed64 100644
> --- a/tools/libxc/xc_sr_common.h
> +++ b/tools/libxc/xc_sr_common.h
> @@ -184,10 +184,12 @@ struct xc_sr_context
>       * migration stream
>       * 0: Plain VM
>       * 1: Remus
> +     * 2: COLO
>       */
>      enum {
>          MIG_STREAM_NONE, /* plain stream */
>          MIG_STREAM_REMUS,
> +        MIG_STREAM_COLO,

I think this shows that the duplicated list (in the comment, above the
enum) is a mistake.  I would prefer it to be removed.

> +/* ===================== colo: common functions ===================== */
> +static void colo_enable_logdirty(libxl__colo_restore_state *crs, libxl__egc *egc)

Here's another missing blank line.  This seems to be a general theme:
can you change this everywhere ?  Thanks.

> @@ -994,6 +1011,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>      const int restore_fd = dcs->restore_fd;
>      libxl__domain_build_state *const state = &dcs->build_state;
>      const int checkpointed_stream = dcs->restore_params.checkpointed_stream;
> +    libxl__colo_restore_state *const crs = &dcs->crs;
> +    libxl_domain_build_info *const info = &d_config->b_info;
>  
>      if (rc) {
>          domcreate_rebuild_done(egc, dcs, rc);
> @@ -1022,6 +1041,13 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>  
>      /* Restore */
>  
> +    /* COLO only supports HVM now */
> +    if (info->type != LIBXL_DOMAIN_TYPE_HVM &&
> +        checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) {
> +        rc = ERROR_FAIL;
> +        goto out;

Please log something here, or it may be very mysterious.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6307b71..48b4858 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -87,6 +87,8 @@
> @@ -3468,7 +3464,6 @@ libxl__stream_read_inuse(const libxl__stream_read_state *stream)
>      return stream->running;
>  }
>  
> -
>  struct libxl__domain_create_state {
>      /* filled in by user */
>      libxl__ao *ao;
> @@ -3484,6 +3479,8 @@ struct libxl__domain_create_state {

Unintentional whitespace change.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel