Re: [PATCH v5 RFC] tools/migrate: Fix regression when migrating from older version of Xen

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

On 26/09/13 16:11, Ian Campbell wrote:
> Just commenting on the API compatibility bits here.
>
> Can you CC Shriram (remus guy) in the next iteration please.
>
> On Thu, 2013-09-26 at 14:45 +0100, Andrew Cooper wrote:
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 4cab294..28af9c2 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -355,6 +355,20 @@
>>   */
>>  #define LIBXL_HAVE_SPICE_VDAGENT 1
>>  
>> +/*
>> + * LIBXL_HAVE_LIBXL_DOMAIN_CREATE_RESTORE_CHECKPOINTED 1
> LIBXL_HAVE_DOMAIN_CREATE... (no need to repeat the LIBXL, or at least we
> don't elsewhere)
>
>> + * If this is defined, libxl_domain_create_restore()'s API has changed to
>> + * include a checkpointed_stream flag which gets passed down to libxc.
>> + */
>> +#define LIBXL_HAVE_LIBXL_DOMAIN_CREATE_RESTORE_CHECKPOINTED 1
>> +#if LIBXL_API_VERSION == 0x040200 || LIBXL_API_VERSION == 0x040300
> <= 0x040300 is fine I think.
>
>> +# define libxl_domain_create_restore libxl_domain_create_restore_V040200
> I prefer "...restore_0x040200" since it looks like the #define.
>
>> +#else
>> +# define libxl_domain_create_restore libxl_domain_create_restore_V040400
>> +#endif
> I think you can omit the #else clause and just call the new function
> libxl_domain_create_restore. libxl will never set LIBXL_API_VERSION when
> building itself.
>
>> +int libxl_domain_create_restore_V040200(
>> +    libxl_ctx *ctx, libxl_domain_config *d_config,
>> +    uint32_t *domid, int restore_fd,
>> +    const libxl_asyncop_how *ao_how,
>> +    const libxl_asyncprogress_how *aop_console_how)
>> +    LIBXL_EXTERNAL_CALLERS_ONLY;
>> +int libxl_domain_create_restore_V040400(
>> +    libxl_ctx *ctx, libxl_domain_config *d_config,
>> +    uint32_t *domid, int restore_fd,
>> +    const libxl_asyncop_how *ao_how,
>> +    const libxl_asyncprogress_how *aop_console_how,
>> +    int checkpointed_stream)
>> +    LIBXL_EXTERNAL_CALLERS_ONLY;
> We seem to habitually put the ao_how and aop_ stuff last, after the
> actual "arguments".
>
> Did we consider adding a libxl_domain_restore_params struct to contain
> the checkpointed flag? That would make this interface more easily
> extensible in the future.

That was considered, but it would involve playing with the IDL (with
which I have no experience), and involve substantially more changes for
both in and out-of-tree consumers of the function.

>
>> +int libxl_domain_create_restore_V040200(
>> +    libxl_ctx *ctx, libxl_domain_config *d_config,
>> +    uint32_t *domid, int restore_fd,
>> +    const libxl_asyncop_how *ao_how,
>> +    const libxl_asyncprogress_how *aop_console_how)
>> +{
>> +    return do_domain_create(ctx, d_config, domid, restore_fd,
>> +                            ao_how, aop_console_how, 0);
>> +}
> Could static inline this as a call to the non suffixed version I think?
>
>

Cannot do both this and drop the #else clause.  (unless I have missed
something obvious)

~Andrew