Re: [Xen-devel] [PATCH v3 18/28] tools/libxl: Infrastructure to convert a legacy stream

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/07/15 15:45, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v3 18/28] tools/libxl: Infrastructure to 
> convert a legacy stream"):
>> Provide a thin wrapper around exec()ing the python conversion utility,
>> and a stub implementation for cases where conversion is not wanted
>> (i.e. not x86).
>>
>> One complication is that the caller of this interface needs to assume
>> ownership of the output fd, to prevent it being closed while still in
>> use in a datacopier.
> ...
>
> Thanks.
>
> I have just very minor comments:
>
>> +int libxl__convert_legacy_stream(libxl__egc *egc,
>> +                                 libxl__conversion_helper_state *chs)
>> +{
> ...
>> +            "--width",
>> +#ifdef __i386__
>> +            "32",
>> +#else
>> +            "64",
>> +#endif
> Firstly, and sorry to not spot this the last time: can we get rid of
> this ifdeffery ?

Not without some way of passing equivalent information.

>
> I'm afraid I don't understand why this script needs to be told the
> toolstack build bit width.  (If indeed the toolstack build bit width
> is the right thing, can't the script tell what bitness it is running
> on, some other way?)

The script needs to know the bitness of the toolstack which saved the
legacy stream, not the bitness of the one which is currently running.

This is because the legacy stream has insufficient information in its
head to determine this with certainty.  The conversion script puts the
onus on the caller to get this correct, rather than possibly
mis-converting a VM without finding a hard error (which is sadly possible).


The ifdefary above is an assumption for the common case.  By
observation, noone outside of XenServer has tried migrating a VM from a
32bit toolstack to a 64bit, or they have and not reported a bug. 
Therefore, the common case is that the bitness of toolstack is the same.

If in the future someone genuinely needs to convert legacy -> v2 and 32
-> 64bit at the same time, they can use the conversion script manually
to achieve this (even in a migration), which will bypass the conversion
logic in libxl, as the incoming stream is already v2.



>
> If we do need to pass in the bitness then there must be some other way
> to find it other than an adhoc #ifdef.
>
> I appreciate that this seems very pernickety for compatibility code
> which will only be executed on i386 or amd64, but #ifdefs like this
> make the code hard to read IMO.

Would it be acceptable to format the above description into a comment?

~Andrew

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