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

4 Feb 2013

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

    

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

Jiri Denemark