Re: [PATCH] tools/many: Fix Generation ID restore interface.

[George Dunlap <george.dunlap@eu.citrix.com>]

On 11/25/2013 08:40 PM, Andrew Cooper wrote:
> The original Generation ID code was submitted before the specification had
> been finalised, and changed completely between the final draft and formal
> release.
>
> Most notably, the size of the Generation ID has doubled to 128 bits, and
> changing it now involves writing a new cryptographically random number in the
> appropriate location, rather than simply incrementing it.
>
> The xc_domain_save() side of the code is fine, but the xc_domain_restore()
> needs substantial changes to be usable.
>
> This patch replaces the old xc_domain_restore() parameters with a new optional
> restore_callback.  If the callback is provided, and an appropriate hunk is
> found in the migration stream, the appropriate piece of guest memory is mapped
> and provided to the callback.
>
> This patch also fixes all the in-tree callers of xc_domain_restore().  All
> callers had the functionality disabled, and never exposed in a public way.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
>
> ---
>
> George:
>
>    Despite this being a feature, I am requesting a freeze exemption.  It is
>    functionality which was not used (or indeed useful) before, and remains that
>    way as far as in-tree consumers are concerned.
>
>    However, as there has been once recent xc_domain_restore() API change, it is
>    less disruptive to other users to do another API change before the 4.4
>    release rather than afterwards.

1. What is the value of this feature?

It's not used and not useful, so as far as I can tell, the value is 0. 
xc_domain_restore() is not a stable interface, so changing it isn't a 
consideration.

2. What is the risk of bugs that may slip the release?  That may not be 
found and ship in the release?

It looks fairly straightforward, but the code it's modifying is 
incredibly fragile, and has a lot of potential combinations which are 
hard to test.

0 value + non-zero risk => I don't see a reason to accept it.

  -George