[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 01/23] xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X



On Thu, Dec 03, 2020 at 02:25:12PM +0000, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> The seemingly arbitrary use of 'pci' and 'pcidev' in the code in libxl_pci.c
> is confusing and also compromises use of some macros used for other device
> types. Indeed it seems that DEFINE_DEVICE_TYPE_STRUCT_X exists solely because
> of this duality.
> 
> This patch purges use of 'pcidev' from the libxl code, allowing evaluation of
> DEFINE_DEVICE_TYPE_STRUCT_X to be replaced with DEFINE_DEVICE_TYPE_STRUCT,
> hence allowing removal of the former.
> 
> For consistency the xl and libs/util code is also modified, but in this case
> it is purely cosmetic.
> 
> NOTE: Some of the more gross formatting errors (such as lack of spaces after
>       keywords) that came into context have been fixed in libxl_pci.c.
> 
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 

This is going to break libxl callers because the name "pcidev" is
visible from the public header.

I agree this is confusing and inconsistent, but we didn't go extra
length to maintain the inconsistency for no reason.

If you really want to change it, I won't stand in the way. In fact, I'm
all for consistency. I think the flag you added should help alleviate
the fallout. Also, you will need to submit patches to libvirt (the only
user I know of) to make use of the flag and switch to the new name and
then request such patch(es) be backported to all maintained version of
libvirt.

See 
https://github.com/libvirt/libvirt/blob/0d05d51b715390e08cd112f83e03b6776412aaeb/src/libxl/libxl_conf.c#L2273

Wei.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.