Re: [PATCH 19/27] tools/libxc+libxl+xl: Restore v2 streams

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

On 16/06/15 15:53, Ian Campbell wrote:
> On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote:
>> @@ -377,6 +384,28 @@ static void record_body_done(libxl__egc *egc,
>>      stream_failed(egc, stream, ret);
>>  }
>>  
>> +void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
>> +                                   int ret, int retval, int errnoval)
>> +{
>> +    libxl__domain_create_state *dcs = dcs_void;
>> +    STATE_AO_GC(dcs->ao);
>> +
>> +    if (ret)
>> +        goto err;
>> +
>> +    if (retval) {
>> +        LOGEV(ERROR, errnoval, "restoring domain");
>> +        ret = ERROR_FAIL;
>> +        goto err;
>> +    }
>> +
>> +    libxl__stream_read_continue(egc, &dcs->srs);
> continue? Is this something to do with checkpointing?

No.  Simply "continue reading records from the stream".

The code has changed since the first iteration, where
libxl__stream_read_continue() was called from outside of this
translation unit.  As it currently stands, I should make it a static
function.

>
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 23f27d4..7418d92 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -346,6 +346,8 @@ libxl_domain_create_info = Struct("domain_create_info",[
>>  
>>  libxl_domain_restore_params = Struct("domain_restore_params", [
> At some point we will need a LIBXL_HAVE #define.
>
>>      ("checkpointed_stream", integer),
>> +    ("stream_version", uint32, {'init_val': '1'}),
> If we aren't going to go for an IDL enum rather than a uint32 you
> probably just want the bare integer 1.
>
> But, I suspect we would prefer an enum, i.e an explicit list of known
> versions, rather than an integer?

Ideally, this should match the version field in the libxl stream header
record.  Realistically, I never expect version 2 needing bumping to
version 3.

>
> I wonder when, if ever, we will be able to flip this to 2? I suppose
> whenever the legacy conversion stuff gets pulled out.
>
>> +    ("legacy_width", uint32),
> From what I've seen so far this is never user provided but is internal
> to libxl and detected[0] at runtime. As such it belongs somewhere else
> other than in the public API.
>
> [0] FVO "detected" == "hardcoded depending on the build arch"

The intention was originally to expose it to the user.  While this is
possible for `xl restore /path/to/legacy/save --was-32bit-toolstack`, it
is much harder for the `xl migrate` case where we cannot control the
sending side.

I am half tempted to say that anyone experiencing 32->64bit problems
should just use the conversion helper themselves.  It is deliberately
written to be usable on the command line as well as automatically.

~Andrew