Re: [PATCH v2 1/3] virhostdev: introduce virHostdevResetAllPCIDevices

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

 





On 7/31/19 9:53 AM, Michal Privoznik wrote:
On 7/23/19 7:35 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@xxxxxxxxx>
---
  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..4dd24a8f65 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -613,6 +613,32 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
      }
  }
  +static int
+virHostdevResetAllPCIDevices(virHostdevManagerPtr mgr,
+                             virPCIDeviceListPtr pcidevs)
+{
+    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();

I don't think this is a good idea. VIR_ERROR() logs the error message, but then virResetLastError() clears the thread local error object and thus if a PCI fails to reset the caller is left with 'uknown error' and sysadmin has to go to logs to find out what's happening. In fact, I think all those virResetLastError() calls should be removed from virHostdevReAttachPCIDevices(). I'll fix it here and in 3/3 too.


Thanks for explaining and fixing it. I blindly moved the code to this new
function (here and in 3/3) without giving too much thought about whether
the existing error handling makes sense.



DHB



+            ret = -1;
+        }
+    }
+
+    return ret;
+}
+

Michal

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux