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@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..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