Simplify PCI hostdev prepare/re-attach using a pciDeviceList type

The qemuPrepareHostDevices() and qemuDomainReAttachHostDevices()
functions are clutter with a bunch of calls to pciGetDevice() and
pciFreeDevice() obscuring the basic logic.

Add a pciDeviceList type and add a qemuGetPciHostDeviceList() function
to build a list from a domain definition. Use this in prepare/re-attach
fto simplify things and eliminate the multiple pciGetDevice calls.

This is especially useful because in the next patch we need to iterate
the hostdevs list a third time and we also need a list type for keeping
track of active devices.

* src/pci.[ch]: add pciDeviceList type and also a per-device 'managed'
  property

* src/libvirt_private.syms: export the new functions

* src/qemu_driver.c: add qemuGetPciHostDeviceList() and re-write
  qemuPrepareHostDevices() and qemuDomainReAttachHostDevices() to use it

src/libvirt_private.syms

@@ -285,7 +285,12 @@ pciFreeDevice;
 pciDettachDevice;
 pciReAttachDevice;
 pciResetDevice;
-
+pciDeviceSetManaged;
+pciDeviceGetManaged;
+pciDeviceListNew;
+pciDeviceListFree;
+pciDeviceListAdd;
+pciDeviceListDel;
 
 # qparams.h
 qparam_get_query;

src/pci.c

@@ -63,6 +63,7 @@ struct _pciDevice {
     unsigned      pci_pm_cap_pos;
     unsigned      has_flr : 1;
     unsigned      has_pm_reset : 1;
+    unsigned      managed : 1;
 };
 
 /* For virReportOOMError()  and virReportSystemError() */
@@ -890,8 +891,116 @@ pciGetDevice(virConnectPtr conn,
 void
 pciFreeDevice(virConnectPtr conn ATTRIBUTE_UNUSED, pciDevice *dev)
 {
+    if (!dev)
+        return;
     VIR_DEBUG("%s %s: freeing", dev->id, dev->name);
     if (dev->fd >= 0)
         close(dev->fd);
     VIR_FREE(dev);
 }
+
+void pciDeviceSetManaged(pciDevice *dev, unsigned managed)
+{
+    dev->managed = !!managed;
+}
+
+unsigned pciDeviceGetManaged(pciDevice *dev)
+{
+    return dev->managed;
+}
+
+pciDeviceList *
+pciDeviceListNew(virConnectPtr conn)
+{
+    pciDeviceList *list;
+
+    if (VIR_ALLOC(list) < 0) {
+        virReportOOMError(conn);
+        return NULL;
+    }
+
+    return list;
+}
+
+void
+pciDeviceListFree(virConnectPtr conn,
+                  pciDeviceList *list)
+{
+    int i;
+
+    if (!list)
+        return;
+
+    for (i = 0; i < list->count; i++) {
+        pciFreeDevice(conn, list->devs[i]);
+        list->devs[i] = NULL;
+    }
+
+    list->count = 0;
+    VIR_FREE(list->devs);
+    VIR_FREE(list);
+}
+
+int
+pciDeviceListAdd(virConnectPtr conn,
+                 pciDeviceList *list,
+                 pciDevice *dev)
+{
+    if (pciDeviceListFind(list, dev)) {
+        pciReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                       _("Device %s is already in use"), dev->name);
+        return -1;
+    }
+
+    if (VIR_REALLOC_N(list->devs, list->count+1) < 0) {
+        virReportOOMError(conn);
+        return -1;
+    }
+
+    list->devs[list->count++] = dev;
+
+    return 0;
+}
+
+void
+pciDeviceListDel(virConnectPtr conn ATTRIBUTE_UNUSED,
+                 pciDeviceList *list,
+                 pciDevice *dev)
+{
+    int i;
+
+    for (i = 0; i < list->count; i++) {
+        if (list->devs[i]->domain   != dev->domain ||
+            list->devs[i]->bus      != dev->bus    ||
+            list->devs[i]->slot     != dev->slot   ||
+            list->devs[i]->function != dev->function)
+            continue;
+
+        pciFreeDevice(conn, list->devs[i]);
+
+        if (i != --list->count)
+            memmove(&list->devs[i],
+                    &list->devs[i+1],
+                    sizeof(*list->devs) * (list->count-i));
+
+        if (VIR_REALLOC_N(list->devs, list->count) < 0) {
+            ; /* not fatal */
+        }
+
+        break;
+    }
+}
+
+pciDevice *
+pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
+{
+    int i;
+
+    for (i = 0; i < list->count; i++)
+        if (list->devs[i]->domain   == dev->domain &&
+            list->devs[i]->bus      == dev->bus    &&
+            list->devs[i]->slot     == dev->slot   &&
+            list->devs[i]->function == dev->function)
+            return list->devs[i];
+    return NULL;
+}

src/pci.h

@@ -27,6 +27,11 @@
 
 typedef struct _pciDevice pciDevice;
 
+typedef struct {
+    unsigned count;
+    pciDevice **devs;
+} pciDeviceList;
+
 pciDevice *pciGetDevice      (virConnectPtr  conn,
                               unsigned       domain,
                               unsigned       bus,
@@ -40,5 +45,20 @@ int        pciReAttachDevice (virConnectPtr  conn,
                               pciDevice     *dev);
 int        pciResetDevice    (virConnectPtr  conn,
                               pciDevice     *dev);
+void      pciDeviceSetManaged(pciDevice     *dev,
+                              unsigned       managed);
+unsigned  pciDeviceGetManaged(pciDevice     *dev);
+
+pciDeviceList *pciDeviceListNew  (virConnectPtr conn);
+void           pciDeviceListFree (virConnectPtr conn,
+                                  pciDeviceList *list);
+int            pciDeviceListAdd  (virConnectPtr conn,
+                                  pciDeviceList *list,
+                                  pciDevice *dev);
+void           pciDeviceListDel  (virConnectPtr conn,
+                                  pciDeviceList *list,
+                                  pciDevice *dev);
+pciDevice *    pciDeviceListFind (pciDeviceList *list,
+                                  pciDevice *dev);
 
 #endif /* __VIR_PCI_H__ */

src/qemu_driver.c

@@ -1329,151 +1329,126 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) {
     return -1;
 }
 
-static int qemuPrepareHostDevices(virConnectPtr conn,
+static pciDeviceList *
-                                  virDomainDefPtr def) {
+qemuGetPciHostDeviceList(virConnectPtr conn,
+                         virDomainDefPtr def)
+{
+    pciDeviceList *list;
     int i;
 
+    if (!(list = pciDeviceListNew(conn)))
+        return NULL;
+
+    for (i = 0 ; i < def->nhostdevs ; i++) {
+        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+        pciDevice *dev;
+
+        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
+            continue;
+        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
+            continue;
+
+        dev = pciGetDevice(conn,
+                           hostdev->source.subsys.u.pci.domain,
+                           hostdev->source.subsys.u.pci.bus,
+                           hostdev->source.subsys.u.pci.slot,
+                           hostdev->source.subsys.u.pci.function);
+        if (!dev) {
+            pciDeviceListFree(conn, list);
+            return NULL;
+        }
+
+        if (pciDeviceListAdd(conn, list, dev) < 0) {
+            pciFreeDevice(conn, dev);
+            pciDeviceListFree(conn, list);
+            return NULL;
+        }
+
+        pciDeviceSetManaged(dev, hostdev->managed);
+    }
+
+    return list;
+}
+
+static int
+qemuPrepareHostDevices(virConnectPtr conn, virDomainDefPtr def)
+{
+    pciDeviceList *pcidevs;
+    int i;
+
+    if (!def->nhostdevs)
+        return 0;
+
+    if (!(pcidevs = qemuGetPciHostDeviceList(conn, def)))
+        return -1;
+
     /* We have to use 2 loops here. *All* devices must
      * be detached before we reset any of them, because
      * in some cases you have to reset the whole PCI,
      * which impacts all devices on it
      */
 
-    for (i = 0 ; i < def->nhostdevs ; i++) {
+    /* XXX validate that non-managed device isn't in use, eg
-        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+     * by checking that device is either un-bound, or bound
+     * to pci-stub.ko
+     */
 
-        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
+    for (i = 0; i < pcidevs->count; i++)
-            continue;
+        if (pciDeviceGetManaged(pcidevs->devs[i]) &&
-        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
+            pciDettachDevice(conn, pcidevs->devs[i]) < 0)
-            continue;
+            goto error;
-
-        if (hostdev->managed) {
-            pciDevice *dev = pciGetDevice(conn,
-                                          hostdev->source.subsys.u.pci.domain,
-                                          hostdev->source.subsys.u.pci.bus,
-                                          hostdev->source.subsys.u.pci.slot,
-                                          hostdev->source.subsys.u.pci.function);
-            if (!dev)
-                goto error;
-
-            if (pciDettachDevice(conn, dev) < 0) {
-                pciFreeDevice(conn, dev);
-                goto error;
-            }
-
-            pciFreeDevice(conn, dev);
-        } /* else {
-             XXX validate that non-managed device isn't in use, eg
-             by checking that device is either un-bound, or bound
-             to pci-stub.ko
-        } */
-    }
 
     /* Now that all the PCI hostdevs have be dettached, we can safely
      * reset them */
-    for (i = 0 ; i < def->nhostdevs ; i++) {
+    for (i = 0; i < pcidevs->count; i++)
-        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+        if (pciResetDevice(conn, pcidevs->devs[i]) < 0)
-        pciDevice *dev;
-
-        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
-            continue;
-        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
-            continue;
-
-        dev = pciGetDevice(conn,
-                           hostdev->source.subsys.u.pci.domain,
-                           hostdev->source.subsys.u.pci.bus,
-                           hostdev->source.subsys.u.pci.slot,
-                           hostdev->source.subsys.u.pci.function);
-        if (!dev)
             goto error;
 
-        if (pciResetDevice(conn, dev) < 0) {
+    pciDeviceListFree(conn, pcidevs);
-            pciFreeDevice(conn, dev);
-            goto error;
-        }
-
-        pciFreeDevice(conn, dev);
-    }
-
     return 0;
 
 error:
+    pciDeviceListFree(conn, pcidevs);
     return -1;
 }
 
 static void
 qemuDomainReAttachHostDevices(virConnectPtr conn, virDomainDefPtr def)
 {
+    pciDeviceList *pcidevs;
     int i;
 
+    if (!def->nhostdevs)
+        return;
+
+    if (!(pcidevs = qemuGetPciHostDeviceList(conn, def))) {
+        virErrorPtr err = virGetLastError();
+        VIR_ERROR(_("Failed to allocate pciDeviceList: %s\n"),
+                  err ? err->message : "");
+        virResetError(err);
+        return;
+    }
+
     /* Again 2 loops; reset all the devices before re-attach */
 
-    for (i = 0 ; i < def->nhostdevs ; i++) {
+    for (i = 0; i < pcidevs->count; i++)
-        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
+        if (pciResetDevice(conn, pcidevs->devs[i]) < 0) {
-        pciDevice *dev;
-
-        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
-            continue;
-        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
-            continue;
-
-        dev = pciGetDevice(conn,
-                           hostdev->source.subsys.u.pci.domain,
-                           hostdev->source.subsys.u.pci.bus,
-                           hostdev->source.subsys.u.pci.slot,
-                           hostdev->source.subsys.u.pci.function);
-        if (!dev) {
-            virErrorPtr err = virGetLastError();
-            VIR_ERROR(_("Failed to allocate pciDevice: %s\n"),
-                      err ? err->message : "");
-            virResetError(err);
-            continue;
-        }
-
-        if (pciResetDevice(conn, dev) < 0) {
             virErrorPtr err = virGetLastError();
             VIR_ERROR(_("Failed to reset PCI device: %s\n"),
                       err ? err->message : "");
             virResetError(err);
         }
 
-        pciFreeDevice(conn, dev);
+    for (i = 0; i < pcidevs->count; i++)
-    }
+        if (pciDeviceGetManaged(pcidevs->devs[i]) &&
-
+            pciReAttachDevice(conn, pcidevs->devs[i]) < 0) {
-    for (i = 0 ; i < def->nhostdevs ; i++) {
-        virDomainHostdevDefPtr hostdev = def->hostdevs[i];
-        pciDevice *dev;
-
-        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
-            continue;
-        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
-            continue;
-        if (!hostdev->managed)
-            continue;
-
-        dev = pciGetDevice(conn,
-                           hostdev->source.subsys.u.pci.domain,
-                           hostdev->source.subsys.u.pci.bus,
-                           hostdev->source.subsys.u.pci.slot,
-                           hostdev->source.subsys.u.pci.function);
-        if (!dev) {
-            virErrorPtr err = virGetLastError();
-            VIR_ERROR(_("Failed to allocate pciDevice: %s\n"),
-                      err ? err->message : "");
-            virResetError(err);
-            continue;
-        }
-
-        if (pciReAttachDevice(conn, dev) < 0) {
             virErrorPtr err = virGetLastError();
             VIR_ERROR(_("Failed to re-attach PCI device: %s\n"),
                       err ? err->message : "");
             virResetError(err);
         }
 
-        pciFreeDevice(conn, dev);
+    pciDeviceListFree(conn, pcidevs);
-    }
 }
 
 static const char *const defaultDeviceACL[] = {