Re: [Qemu-devel] [PATCH 1/3] pci: move check for existing devfn into new pci_bus_devfn_available() helper

[Marcel Apfelbaum]

On 07/07/2017 10:44, Mark Cave-Ayland wrote:

Also touch up the logic in do_pci_register_device() accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
---
  hw/pci/pci.c |   14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0c6f74a..04e6edb 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -951,6 +951,15 @@ uint16_t pci_requester_id(PCIDevice *dev)
      return pci_req_id_cache_extract(&dev->requester_id_cache);
  }

Hi Mark,

+static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
+{
+    if (bus->devices[devfn]) {
+        return false;
+    } else {
+        return true;
+    }
+}
+

The function may simply return bus->devices[devfn], right?
(the return type should take care of the rest)

  /* -1 for devfn means auto assign */
  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                           const char *name, int devfn,
@@ -974,14 +983,15 @@ static PCIDevice *do_pci_register_device(PCIDevice 
*pci_dev, PCIBus *bus,
      if (devfn < 0) {
          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
              devfn += PCI_FUNC_MAX) {
-            if (!bus->devices[devfn])
+            if (pci_bus_devfn_available(bus, devfn)) {

I am all for making the code more readable, but in this
case I am not sure if it worth it. "bus->devices[devfn]"
is self explanatory, but maybe is a matter of taste.

Thanks,
Marcel

                  goto found;
+            }
          }
          error_setg(errp, "PCI: no slot/function available for %s, all in use",
                     name);
          return NULL;
      found: ;
-    } else if (bus->devices[devfn]) {
+    } else if (!pci_bus_devfn_available(bus, devfn)) {
          error_setg(errp, "PCI: slot %d function %d not available for %s,"
                     " in use by %s",
                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,