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

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 06/30/2015 06:42 PM, Ian Campbell wrote:
> 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?

Yes, it is the negotiation record for COLO.

>
>> 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).

I will leave this to Andy.

>
>> +
>> +     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?

Yes, but the meaning is diffrent, the checkpoint record means the
end of a checkpoint, this means a new checkpoint is needed.

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

-- 
Thanks,
Yang.