Re: [PATCH v3 COLOPre 23/26] docs/libxl: Introduce COLO_CONTEXT to support migration v2 colo streams

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

On Thu, 2015-06-25 at 14:25 +0800, Yang Hongyang wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>

This seems to mix up forward and backward facing information in a single
record?

> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  docs/specs/libxl-migration-stream.pandoc | 21 ++++++++++++++++++++-
>  tools/libxl/libxl_sr_stream_format.h     | 11 +++++++++++
>  tools/python/xen/migration/libxl.py      |  9 +++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/specs/libxl-migration-stream.pandoc b/docs/specs/libxl-migration-stream.pandoc
> index d41932a..7e1edaa 100644
> --- a/docs/specs/libxl-migration-stream.pandoc
> +++ b/docs/specs/libxl-migration-stream.pandoc
> @@ -121,7 +121,9 @@ type         0x00000000: END
>  
>               0x00000004: CHECKPOINT_END
>  
> -             0x00000005 - 0x7FFFFFFF: Reserved for future _mandatory_
> +             0x00000005: COLO_CONTEXT
> +
> +             0x00000006 - 0x7FFFFFFF: Reserved for future _mandatory_
>               records.
>  
>               0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
> @@ -216,3 +218,20 @@ A checkpoint end record marks the end of a checkpoint in the image.
>  
>  The end record contains no fields; its body_length is 0.
>  
> +COLO\_CONTEXT
> +--------------
> +
> +A COLO context record contains the control information for COLO.

I don't know what Andy thinks, but this seems like a rather generic
catch-all record type to me. It would seem better to have one or more
more concrete records for different aspects (e.g. status of the
secondary VM, although that seems like a backchannel thing too and as I
mentioned before I'm not sure those should be interleaved in the spec in
this way).

> +
> +     0     1     2     3     4     5     6     7 octet
> +    +------------------------+------------------------+
> +    | control_id             | padding                |
> +    +------------------------+------------------------+
> +
> +--------------------------------------------------------------------
> +Field            Description
> +------------     ---------------------------------------------------
> +control_id       0x00000000: New checkpoint

I think we already have a checkpoint record type, don't we?


> +                 0x00000001: Secondary VM is suspended
> +                 0x00000002: Secondary VM is ready
> +                 0x00000003: Secondary VM is resumed