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

Re: [libvirt] [PATCH v3 08/13] Rename all PCI device functions to have a standard name prefix



On Fri, Feb 01, 2013 at 11:18:30 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Rename all the pciDeviceXXX and pciXXXDevice APIs to have a
> fixed virPCIDevice name prefix

Some functions gained just virPCI prefix, I guess that means they don't
take virPCIDevicePtr arguments. In any case, the shorter prefix the
better so I'm not opposed to it :-)

...
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 1b8a9cd..b5d7c5e 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
...
> @@ -856,7 +856,7 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
>                                                     hostdevs,
>                                                     nhostdevs))) {
>          virErrorPtr err = virGetLastError();
> -        VIR_ERROR(_("Failed to allocate pciDeviceList: %s"),
> +        VIR_ERROR(_("Failed to allocate virPCIDeviceList: %s"),

Why not just "PCI device list"?

>                    err ? err->message : _("unknown error"));
>          virResetError(err);
>          goto cleanup;
...
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 0fb9923..695f372 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
...
> @@ -748,39 +748,39 @@ pciTryPowerManagementReset(pciDevice *dev, int cfgfd)
>  }
>  
>  static int
> -pciInitDevice(pciDevice *dev, int cfgfd)
> +virPCIDeviceInitDevice(virPCIDevicePtr dev, int cfgfd)

Why not just virPCIDeviceInit?

>  {
>      int flr;
>  
> -    dev->pcie_cap_pos   = pciFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP);
> -    dev->pci_pm_cap_pos = pciFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM);
> -    flr = pciDetectFunctionLevelReset(dev, cfgfd);
> +    dev->pcie_cap_pos   = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_EXP);
> +    dev->pci_pm_cap_pos = virPCIDeviceFindCapabilityOffset(dev, cfgfd, PCI_CAP_ID_PM);
> +    flr = virPCIDeviceDetectFunctionLevelReset(dev, cfgfd);
>      if (flr < 0)
>          return flr;
>      dev->has_flr        = flr;
> -    dev->has_pm_reset   = pciDetectPowerManagementReset(dev, cfgfd);
> +    dev->has_pm_reset   = virPCIDeviceDetectPowerManagementReset(dev, cfgfd);
>  
>      return 0;
>  }
...
> @@ -887,13 +887,13 @@ recheck:
>  }
>  
>  static int
> -pciUnbindDeviceFromStub(pciDevice *dev, const char *driver)
> +virPCIDeviceUnbindDeviceFromStub(virPCIDevicePtr dev, const char *driver)

virPCIDeviceUnbindFromStub would be a much better name.

>  {
>      int result = -1;
>      char *drvdir = NULL;
>      char *path = NULL;
>  
> -    if (pciDriverDir(&drvdir, driver) < 0)
> +    if (virPCIDriverDir(&drvdir, driver) < 0)
>          goto cleanup;
>  
>      if (!dev->unbind_from_stub)
...
> @@ -975,7 +975,7 @@ cleanup:
>  
>  
>  static int
> -pciBindDeviceToStub(pciDevice *dev, const char *driver)
> +virPCIDeviceBindDeviceToStub(virPCIDevicePtr dev, const char *driver)

virPCIDeviceBindToStub sounds better.

>  {
>      int result = -1;
>      char *drvdir = NULL;
...
> @@ -1118,36 +1118,36 @@ cleanup:
>      VIR_FREE(path);
>  
>      if (result < 0) {
> -        pciUnbindDeviceFromStub(dev, driver);
> +        virPCIDeviceUnbindDeviceFromStub(dev, driver);
>      }
>  
>      return result;
>  }
>  
>  int
> -pciDettachDevice(pciDevice *dev,
> -                 pciDeviceList *activeDevs,
> -                 pciDeviceList *inactiveDevs,
> -                 const char *driver)
> +virPCIDeviceDettach(virPCIDevicePtr dev,
> +                    virPCIDeviceList *activeDevs,
> +                    virPCIDeviceList *inactiveDevs,
> +                    const char *driver)

Since you're already changing the function name, you could have fixed
the typo: virPCIDeviceDetach.

>  {
> -    if (pciProbeStubDriver(driver) < 0) {
> +    if (virPCIProbeStubDriver(driver) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to load PCI stub module %s"), driver);
>          return -1;
>      }
...
> @@ -1155,29 +1155,29 @@ pciDettachDevice(pciDevice *dev,
>  }
>  
>  int
> -pciReAttachDevice(pciDevice *dev,
> -                  pciDeviceList *activeDevs,
> -                  pciDeviceList *inactiveDevs,
> -                  const char *driver)
> +virPCIDeviceReAttach(virPCIDevicePtr dev,
> +                     virPCIDeviceListPtr activeDevs,
> +                     virPCIDeviceListPtr inactiveDevs,
> +                     const char *driver)

I'd make the "A" lower case.

>  {
> -    if (pciProbeStubDriver(driver) < 0) {
> +    if (virPCIProbeStubDriver(driver) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to load PCI stub module %s"), driver);
>          return -1;
>      }
...
> @@ -1292,12 +1292,12 @@ pciWaitForDeviceCleanup(pciDevice *dev, const char *matcher)
>  }
>  
>  static char *
> -pciReadDeviceID(pciDevice *dev, const char *id_name)
> +virPCIDeviceReadDeviceID(virPCIDevicePtr dev, const char *id_name)

Would virPCIDeviceReadID be better? I'm not sure but DeviceReadDeviceID
is weird.

>  {
>      char *path = NULL;
>      char *id_str;
>  
> -    if (pciDeviceFile(&path, dev->name, id_name) < 0) {
> +    if (virPCIFile(&path, dev->name, id_name) < 0) {
>          return NULL;
>      }
>  
...
> @@ -1880,8 +1880,8 @@ out:
>  }
>  
>  static int
> -pciGetPciConfigAddressFromSysfsDeviceLink(const char *device_link,
> -                                          struct pci_config_address **bdf)
> +virPCIGetDeviceAddressFromSysfsDeviceLink(const char *device_link,
> +                                          virPCIDeviceAddressPtr *bdf)

virPCIGetDeviceAddressFromSysfsLink would sound a bit better to me.

>  {
>      char *config_address = NULL;
>      char *device_path = NULL;
...

ACK whether you implement changes I suggested or not (or just some of
them) as long as make all check syntax-check succeeds.

Jirka


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