Re: [PATCH 07/29] docs: libxl migration stream specification

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/09/14 11:45, Ian Campbell wrote:
> On Wed, 2014-09-10 at 18:10 +0100, Andrew Cooper wrote:
>
> Please run a spell checker over this ("responsibile" was the first one I
> spotted, "resonsible" was the second)
>
>> +Purpose
>> +-------
> I think this (and the equivalent section in the libxc level doc) should
> be moved to the commit log. It's useful right now as a motivator for the
> change but in a years time it will just be some random fluff in a doc
> that everyone has to page past to get to the interesting bits.

Ok.

>
>> +This design addresses the above points, allowing for a completely
>> +self-contained, extensible stream with each layer responsibile for its own
>> +appropriate information.
>> +
>> +
>> +Not Yet Included
>> +----------------
>> +
>> +The following features are not yet fully specified and will be
>> +included in a future draft.
>> +
>> +* Remus
>> +
>> +* ARM
>> +
>> +
>> +Overview
>> +========
>> +
>> +The image format consists of a _Header_, followed by 1 or more _Records_.
>> +Each record consists of a type and length field, followed by any type-specific
>> +data.
>> +
>> +\clearpage
>> +
>> +Header
>> +======
>> +
>> +The header identifies the stream as a `libxl` stream, including the version of
>> +this specification that it complies with.
>> +
>> +All fields in this header shall be in _big-endian_ byte order, regardless of
>> +the setting of the endianness bit.
>> +
>> +     0     1     2     3     4     5     6     7 octet
>> +    +-------------------------------------------------+
>> +    | ident                                           |
>> +    +-----------------------+-------------------------+
>> +    | version               | options                 |
>> +    +-----------------------+-------------------------+
>> +
>> +--------------------------------------------------------------------
>> +Field       Description
>> +----------- --------------------------------------------------------
>> +ident       0x4c6962786c466d74 ("LibxlFmt" in ASCII).
>> +
>> +version     0x00000002.  The version of this specification.
>> +
>> +options     bit 0: Endianness.    0 = little-endian, 1 = big-endian.
>> +
>> +            bit 1: Legacy Format. If set, this stream was created by
>> +                                  the legacy conversion tool.
> Are such streams otherwise distinguishable from a stream which was
> created directly? Should anything care about this?
>
> I fear this is going to be used to paper over shortcomings in the
> conversion tool somehow, but I suppose I'll see later in the series.

My concern here was regarding the d_config.  A legacy converted stream
cannot possibly contain a domain_json blob, so must have a d_config
passed by the caller.  Admittedly, this did pre-date realising that
libxl currently allows the caller to blindly overwrite the config
anyway, and this needs to continue for compatibility reasons.

However, knowing that a stream has been converted is a key debugging
detail, even if this flag serves no other purpose from libxl's point of
view.

>
>> +LIBXC\_CONTEXT
>> +--------------
>> +
>> +A libxc context record is a marker, indicating that the stream should be
>> +handed to `xc_domain_restore()`.  `libxc` shall be resonsible for reading its
>> +own image format from the stream.
>> +
>> +     0     1     2     3     4     5     6     7 octet
>> +    +-------------------------------------------------+
>> +
>> +The libxc context record contains no fields; its body_length is 0[^1].
>> +
>> +
>> +[^1]: The sending side cannot calculate ahead of time how much data `libxc`
>> +might write into the stream, especially for live migration where the quantity
>> +of data is partially proportional to the elapsed time.
> I think this deserves to be in the main text and not a footnote
> (assuming that's what ^1 is).

^1 is indeed a footnote.

>  I think it should probably be expanded to
> explain how a toolstack can actually treat this, which I assume is to
> assume that xc_domain_restore will consume exactly its own business and
> then return.

Ok

>
> Something somewhere also ought to say what libxc will have done on
> error, which is presumably to have left the stream in some indeterminate
> state and almost certainly not at the next libxl record boundary.

Correct - I shall discuss this.

>
>> +
>> +XENSTORE\_DATA
>> +-------------
>> +
>> +A record containing xenstore key/value pairs of data.
> In what format?

Ah, yes.  "Whatever libxl currently does", although I guess I need to
expand on that.

>
>> +     0     1     2     3     4     5     6     7 octet
>> +    +-------------------------------------------------+
>> +    | xenstore key/value pairs                        |
>> +    ...
>> +    +-------------------------------------------------+
>> +
> Ian.
>