RE: [PATCH v4 01/23] xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
> Sent: 01 December 2020 12:33
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Ian Jackson <iwj@xenproject.org>; Wei Liu <wl@xen.org>;
> Anthony PERARD <anthony.perard@citrix.com>
> Subject: Re: [PATCH v4 01/23] xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X
> 
> Hi, Paul!
> 
> On 11/24/20 10:01 AM, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > 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@amazon.com>
> > ---
> > Cc: Ian Jackson <iwj@xenproject.org>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >   tools/include/libxl.h             |  17 +-
> >   tools/libs/light/libxl_create.c   |   6 +-
> >   tools/libs/light/libxl_dm.c       |  18 +-
> >   tools/libs/light/libxl_internal.h |  45 ++-
> >   tools/libs/light/libxl_pci.c      | 582 +++++++++++++++++++-------------------
> >   tools/libs/light/libxl_types.idl  |   2 +-
> >   tools/libs/util/libxlu_pci.c      |  36 +--
> >   tools/xl/xl_parse.c               |  28 +-
> >   tools/xl/xl_pci.c                 |  68 ++---
> >   tools/xl/xl_sxp.c                 |  12 +-
> >   10 files changed, 409 insertions(+), 405 deletions(-)
> >
> > diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> > index 1ea5b4f446..fbe4c81ba5 100644
> > --- a/tools/include/libxl.h
> > +++ b/tools/include/libxl.h
> > @@ -445,6 +445,13 @@
> >   #define LIBXL_HAVE_DISK_SAFE_REMOVE 1
> >
> [snip]
> > -/* Scan through /sys/.../pciback/slots looking for pcidev's BDF */
> > -static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev)
> > +/* Scan through /sys/.../pciback/slots looking for pci's BDF */
> > +static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pci)
> >   {
> >       FILE *f;
> >       int rc = 0;
> > @@ -635,11 +635,11 @@ static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev)
> >           return ERROR_FAIL;
> >       }
> >
> > -    while(fscanf(f, "%x:%x:%x.%d\n", &dom, &bus, &dev, &func)==4) {
> > -        if(dom == pcidev->domain
> > -           && bus == pcidev->bus
> > -           && dev == pcidev->dev
> > -           && func == pcidev->func) {
> > +    while (fscanf(f, "%x:%x:%x.%d\n", &dom, &bus, &dev, &func)==4) {
> So, then you can probably put spaces around "4" if touching this line

Oh yes. Will do.

> > +        if (dom == pci->domain
> > +            && bus == pci->bus
> > +            && dev == pci->dev
> > +            && func == pci->func) {
> >               rc = 1;
> >               goto out;
> >           }
> > @@ -649,7 +649,7 @@ out:
> >       return rc;
> >   }
> >
> 
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Thanks,

  Paul