Re: [PATCH v2] tools/xl: correctly shows split eventchannel for netfront

[annie li <annie.li@oracle.com>]

On 2014/1/24 18:58, Ian Campbell wrote:
> On Fri, 2014-01-24 at 18:51 +0800, Annie Li wrote:
>
>> +/*
>> + * LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
>> + * If this is defined, libxl_nicinfo will contain an integer type field: evtch_rx,
>> + * containing the value of eventchannel for rx path of netback&netfront which support
>> + * split event channel. The original evtch field contains the value of eventchannel
>> + * for tx path in such case.
> I think it can be either "event channel" (two words) or
> "evtchn" (abbreviation) but not "eventchannel".

Ok, thanks.

>
>> + *
>> + */
>> +#if LIBXL_API_VERSION > 0x040400
>> +#define LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL 1
> This should be unconditional. There are several existing examples in
> libxl.h which you could have copied.

OK

>
>> +#endif
>> +
>>   /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>>    * called from within libxl itself. Callers outside libxl, who
>>    * do not #include libxl_internal.h, are fine. */
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 649ce50..b1b4946 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -489,6 +489,9 @@ libxl_nicinfo = Struct("nicinfo", [
>>       ("devid", libxl_devid),
>>       ("state", integer),
>>       ("evtch", integer),
>> +#ifdef LIBXL_HAVE_NETWORK_SPLIT_EVENTCHANNEL
>> +    ("evtch_rx", integer),
>> +#endif
> Did you even build this? Because it can't have worked (Libxl_types.idl
> is a Python source file which is not preprocessed).

My fault, I only built out xl from libxl and replace current one for 
test, not familiar with Python.:-(

>
> In any case, this ifdef is unneccessary and wrong. the LIBXL_HAVE
> indicates to the consumer that the field is present, but it should not
> actually gate the presence of the field.
>
> Think about it -- if an application is building against libxl version
> 4.4 (which was therefore built with evtchn_rx present) but requests
> LIBXL_API_VERSION == 0x040300 then your patch has now created an ABI
> mismatch.

I misunderstood LIBXL_HAVE_* in libxl.h, and thought it is used by libxl 
itself. Much clear now, thanks.

Thanks
Annie