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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant <xadimgnik@xxxxxxxxx>
> Sent: 07 December 2020 16:18
> To: paul@xxxxxxx; 'Wei Liu' <wl@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; 
> 'Oleksandr Andrushchenko'
> <oleksandr_andrushchenko@xxxxxxxx>; 'Ian Jackson' <iwj@xxxxxxxxxxxxxx>; 
> 'Anthony PERARD'
> <anthony.perard@xxxxxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH v5 01/23] xl / libxl: s/pcidev/pci and remove
> DEFINE_DEVICE_TYPE_STRUCT_X
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> > -----Original Message-----
> [snip]
> > > > >
> > > > > 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.
> > > >
> > > > Yes, I thought that was the idea... we can make API changes if we add a 
> > > > flag. I could see about
> > > adding shims to translate the names
> > > > and keep the internal code clean.
> > >
> > > Yes if you can add some internal shims to handle it that would be
> > > great. Otherwise you will need to at least fix libvirt.
> > >
> >
> > I think shims are safest. We don't know what other callers are lurking out 
> > there :-)
> >
> 
> Wei,
> 
> Looking at this again; the only mentions of 'pcidev' in the public header 
> that I can see are in
> argument names in function
> prototypes, modified in the following hunks.
> 
> @@ -2307,15 +2314,15 @@ int libxl_device_pvcallsif_destroy(libxl_ctx *ctx, 
> uint32_t domid,
> 
>  /* PCI Passthrough */
>  int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid,
> -                         libxl_device_pci *pcidev,
> +                         libxl_device_pci *pci,
>                           const libxl_asyncop_how *ao_how)
>                           LIBXL_EXTERNAL_CALLERS_ONLY;
>  int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid,
> -                            libxl_device_pci *pcidev,
> +                            libxl_device_pci *pci,
>                              const libxl_asyncop_how *ao_how)
>                              LIBXL_EXTERNAL_CALLERS_ONLY;
>  int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid,
> -                             libxl_device_pci *pcidev,
> +                             libxl_device_pci *pci,
>                               const libxl_asyncop_how *ao_how)
>                               LIBXL_EXTERNAL_CALLERS_ONLY;
> 
> @@ -2359,8 +2366,8 @@ int libxl_device_events_handler(libxl_ctx *ctx,
>   * added or is not bound, the functions will emit a warning but return
>   * SUCCESS.
>   */
> -int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci 
> *pcidev, int rebind);
> -int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci 
> *pcidev, int rebind);
> +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pci, 
> int rebind);
> +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci 
> *pci, int rebind);
>  libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
> 
>  /* CPUID handling */
> 
> I can't see how renaming these will break anything. The type name (which is 
> what I thought I'd
> changed) actually remains the same.
> The main changes are in the libxl__device_type structure but AFAICT that is 
> not publicly visible. Am I
> missing something?

Oh NM... I see the direct use of the domain_config field names lower down. I 
guess I can probably leave those names alone.

  Paul

> 
>   Paul