Re: [Xen-devel] [PATCH v11 07/27] docs/libxl: Introduce CHECKPOINT_CONTEXT to support migration v2 colo streams

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Changlong Xie writes ("[PATCH v11 07/27] docs/libxl: Introduce 
CHECKPOINT_CONTEXT to support migration v2 colo streams"):
> From: Wen Congyang <wency@xxxxxxxxxxxxxx>

I think we will want to see an ack from Andy Cooper on this, in due
course.

> It is the negotiation record for COLO.
> Primary->Secondary:
> control_id      0x00000000: Secondary VM is out of sync, start a new 
> checkpoint
> Secondary->Primary:
>                 0x00000001: Secondary VM is suspended
>                 0x00000002: Secondary VM is ready
>                 0x00000003: Secondary VM is resumed

I don't think it is necessary to repeat the enum assignment here in
the commit message.


> +CHECKPOINT\_STATE
> +--------------

This documentation patch ought to explicitly mention COLO, and have
cross-references to the various documents (eg, the README added in the
previous patch).

> +A checkpoint state record contains the control information for checkpoint.
> +
> +     0     1     2     3     4     5     6     7 octet
> +    +------------------------+------------------------+
> +    | control_id             | padding                |
> +    +------------------------+------------------------+
> +
> +--------------------------------------------------------------------
> +Field            Description
> +------------     ---------------------------------------------------
> +control_id       0x00000000: Secondary VM is out of sync, start a new 
> checkpoint
> +                 (Primary -> Secondary)
> +
> +                 0x00000001: Secondary VM is suspended (Secondary -> Primary)
> +
> +                 0x00000002: Secondary VM is ready (Secondary -> Primary)
> +
> +                 0x00000003: Secondary VM is resumed (Secondary -> Primary)

I think this should be accompanied by an explanation of what order
these messages are sent in, and what both ends may or may not do
during that time.


> @@ -212,6 +214,11 @@ class VerifyLibxl(VerifyBase):
>          if len(content) != 0:
>              raise RecordError("Checkpoint end record with non-zero length")
>  
> +    def verify_record_checkpoint_state(self, content):
> +        """ Checkpoint state """
> +        if len(content) == 0:
> +            raise RecordError("Checkpoint state record with zero length")
> +

I'm not verify familiar with this area of the code, but I think that
this should probably check that the control_id is as expected.  Can it
know what the right sequencing is ?

Ian.

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