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

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(a)gmail.com&gt; > --- >   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.

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