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 at 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