This code that executes virPCIDeviceReset in all virPCIDevicePtr
objects of a given virPCIDeviceListPtr list is replicated twice
in the code. Putting it in a helper function helps with
readability.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/util/virhostdev.c | 54 +++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index a3647a6cf4..7cb0beb545 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
     }
 }
 
+static int
+virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs,
+                             virHostdevManagerPtr mgr)
+{
+    int ret = 0;
+    size_t i;
+
+    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
+        virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
+
+        /* We can avoid looking up the actual device here, because performing
+         * a PCI reset on a device doesn't require any information other than
+         * the address, which 'pci' already contains */
+        VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
+        if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
+                              mgr->inactivePCIHostdevs) < 0) {
+            VIR_ERROR(_("Failed to reset PCI device: %s"),
+                      virGetLastErrorMessage());
+            virResetLastError();
+            ret = -1;
+        }
+    }
+
+    return ret;
+}
+
 int
 virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
                             const char *drv_name,
@@ -765,17 +791,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
 
     /* Step 3: Now that all the PCI hostdevs have been detached, we
      * can safely reset them */
-    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
-        virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-
-        /* We can avoid looking up the actual device here, because performing
-         * a PCI reset on a device doesn't require any information other than
-         * the address, which 'pci' already contains */
-        VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
-        if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
-                              mgr->inactivePCIHostdevs) < 0)
-            goto reattachdevs;
-    }
+    if (virHostdevResetAllPCIDevices(pcidevs, mgr) < 0)
+        goto reattachdevs;
 
     /* Step 4: For SRIOV network devices, Now that we have detached the
      * the network device, set the new netdev config */
@@ -1046,20 +1063,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr mgr,
     }
 
     /* Step 4: perform a PCI Reset on all devices */
-    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
-        virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
-
-        /* We can avoid looking up the actual device here, because performing
-         * a PCI reset on a device doesn't require any information other than
-         * the address, which 'pci' already contains */
-        VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
-        if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
-                              mgr->inactivePCIHostdevs) < 0) {
-            VIR_ERROR(_("Failed to reset PCI device: %s"),
-                      virGetLastErrorMessage());
-            virResetLastError();
-        }
-    }
+    virHostdevResetAllPCIDevices(pcidevs, mgr);
 
     /* Step 5: Reattach managed devices to their host drivers; unmanaged
      *         devices don't need to be processed further */
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
> This code that executes virPCIDeviceReset in all virPCIDevicePtr
> objects of a given virPCIDeviceListPtr list is replicated twice
> in the code. Putting it in a helper function helps with
> readability.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   src/util/virhostdev.c | 54 +++++++++++++++++++++++--------------------
>   1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index a3647a6cf4..7cb0beb545 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
>       }
>   }
>   
> +static int
> +virHostdevResetAllPCIDevices(virPCIDeviceListPtr pcidevs,
> +                             virHostdevManagerPtr mgr)

Small nit pick here, I prefer arguments appearing in their order of 
importance. That means, virHostdevManager is more important than the 
virPCIDeviceList, because the manager is built on the top of virpci module.

> +{
> +    int ret = 0;
> +    size_t i;
> +
> +    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> +        virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
> +
> +        /* We can avoid looking up the actual device here, because performing
> +         * a PCI reset on a device doesn't require any information other than
> +         * the address, which 'pci' already contains */
> +        VIR_DEBUG("Resetting PCI device %s", virPCIDeviceGetName(pci));
> +        if (virPCIDeviceReset(pci, mgr->activePCIHostdevs,
> +                              mgr->inactivePCIHostdevs) < 0) {
> +            VIR_ERROR(_("Failed to reset PCI device: %s"),
> +                      virGetLastErrorMessage());
> +            virResetLastError();
> +            ret = -1;
> +        }
> +    }
> +
> +    return ret;
> +}
> +

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list