Re: [Xen-devel] [PATCH v4 0/9] Migration Stream v2

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-04-30 at 19:36 +0100, Andrew Cooper wrote:
> There are certainly some areas still in need improvement.  There are plans to
> combine the save logic for PV and HVM guests by adding a few more hooks to
> save_restore_ops.  While reviewing the series for posting, I have noticed some
> accidental duplication from attempting to merge PV and HVM together
> (ctx->save.p2m_size and ctx->x86_pv.max_pfn) which can be consolidated.

Overall it looks good, well done to all those involved.

A few common issues I spotted which I didn't repeat every on every
instance:
      * Magic numbers (some with prexisting suitable defines) should
        msotly be #defined, or 
      * Chaining of unrelated things into if/else/else (especially where
        mixing input validation and actual functionality as I think I
        saw at least once)
      * { for a struct initialiser on the same line as the variable.
      * Apostrophes in comments are so last year

I think that was it (but if you notice me commenting on something
repeatedly but not consistently there's a good chance I meant to
remember it for this list).

> Moving forward will be work to do with converting a legacy image to a v2
> image.
> 
> Questions/issues needing discussing are:
> 
> 1) What level should the conversion happen.  The v2 format is capable of
>    detecting a legacy stream, but libxc is arguably too low level for the
>    conversion decision to happen.

The detection necessarily has to read the first N bytes, doesn't it?
Which pretty much means the decision has to be made in libxc I think?

> 2) What to do about the layer violations which is the toolstack record and
>    device model record.  Libxl for HVM guests is the only known user of the
>    'toolstack' chunk, which contains some xenstore key/value pairs.  This data
>    should not be a blob in the middle of the libxc layer, and should be
>    promoted to a first class member of a libxl migration stream.

Stefano added this, I can't remember what it is or why it ended up in
libxc, it could well just be "libxc_domain_save is unhackable", or maybe
there was a reason.

Ian.


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