[libvirt] [PATCH v1 3/3] virhostdev: remove virHostdevReattachPCIDevice

[Daniel Henrique Barboza]

On 7/19/19 8:52 AM, Michal Privoznik wrote:
> On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
>> We have 3 pieces of code that do slightly the same thing, but
>> it varies depending on where it is called:
>>
>> - virPCIDeviceReattach(). This is where the actual re-attach
>> work happens;
>>
>> - virHostdevReattachPCIDevice(). This is a static function from
>> virhostdev.c that calls virPCIDeviceReattach() after waiting
>> for device cleanup for the KVM PCI stub driver;
>>
>> - virHostdevPCINodeDeviceReAttach(). This function also
>> calls virPCIDeviceReattach(), but setting some device properties
>> beforehand.
>>
>> All these extra operations that are done before virPCIDeviceReattach()
>> can be moved inside the function itself,  allowing for the
>> same re-attach behavior everywhere.
>>
>> This patch consolidates all these pre-conditions inside the
>> body of virPCIDeviceReattach(). With that, virHostdevReattachPCIDevice()
>> can be removed and the callers can use virPCIDeviceReattach()
>> directly instead.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   src/util/virhostdev.c | 43 +++++++++----------------------------------
>>   src/util/virpci.c     | 29 ++++++++++++++++++++++++++---
>>   2 files changed, 35 insertions(+), 37 deletions(-)
>>
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index 53aacb59b4..23be037a39 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
>> @@ -613,33 +613,6 @@ 
>> virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev,
>>       }
>>   }
>>   -/*
>> - * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
>> - * are locked
>> - */
>> -static void
>> -virHostdevReattachPCIDevice(virHostdevManagerPtr mgr,
>> -                            virPCIDevicePtr actual)
>> -{
>> -    /* Wait for device cleanup if it is qemu/kvm */
>> -    if (virPCIDeviceGetStubDriver(actual) == VIR_PCI_STUB_DRIVER_KVM) {
>> -        int retries = 100;
>> -        while (virPCIDeviceWaitForCleanup(actual, 
>> "kvm_assigned_device")
>> -               && retries) {
>> -            usleep(100*1000);
>> -            retries--;
>> -        }
>> -    }
>> -
>> -    VIR_DEBUG("Reattaching PCI device %s", 
>> virPCIDeviceGetName(actual));
>> -    if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
>> -                             mgr->inactivePCIHostdevs) < 0) {
>> -        VIR_ERROR(_("Failed to re-attach PCI device: %s"),
>> -                  virGetLastErrorMessage());
>> -        virResetLastError();
>> -    }
>> -}
>> -
>>   static void
>>   virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
>>                                   virHostdevManagerPtr mgr)
>> @@ -655,11 +628,17 @@ 
>> virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
>>           if (!(actual = 
>> virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
>>               continue;
>>   -        if (virPCIDeviceGetManaged(actual))
>> -            virHostdevReattachPCIDevice(mgr, actual);
>> -        else
>> +        if (virPCIDeviceGetManaged(actual)) {
>> +            if (virPCIDeviceReattach(actual, mgr->activePCIHostdevs,
>> + mgr->inactivePCIHostdevs) < 0) {
>> +                VIR_ERROR(_("Failed to re-attach PCI device: %s"),
>> +                          virGetLastErrorMessage());
>> +                virResetLastError();
>> +            }
>> +        } else {
>>               VIR_DEBUG("Not reattaching unmanaged PCI device %s",
>>                         virPCIDeviceGetName(actual));
>> +        }
>>       }
>>   }
>>   @@ -2050,10 +2029,6 @@ 
>> virHostdevPCINodeDeviceReAttach(virHostdevManagerPtr mgr,
>>       if (virHostdevIsPCINodeDeviceUsed(virPCIDeviceGetAddress(pci), 
>> &data))
>>           goto cleanup;
>>   -    virPCIDeviceSetUnbindFromStub(pci, true);
>> -    virPCIDeviceSetRemoveSlot(pci, true);
>> -    virPCIDeviceSetReprobe(pci, true);
>> -
>
> The real change here, is that these lines ^^
>
>>       if (virPCIDeviceReattach(pci, mgr->activePCIHostdevs,
>>                                mgr->inactivePCIHostdevs) < 0)
>>           goto cleanup;
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index 75e8daadd5..4594643d3c 100644
>> --- a/src/util/virpci.c
>> +++ b/src/util/virpci.c
>> @@ -1509,19 +1509,39 @@ virPCIDeviceDetach(virPCIDevicePtr dev,
>>       return 0;
>>   }
>>   +/*
>> + * Pre-condition: inactivePCIHostdevs & activePCIHostdevs
>> + * are locked
>> + */
>>   int
>>   virPCIDeviceReattach(virPCIDevicePtr dev,
>>                        virPCIDeviceListPtr activeDevs,
>>                        virPCIDeviceListPtr inactiveDevs)
>>   {
>> +    int ret = -1;
>> +
>>       if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) {
>>           virReportError(VIR_ERR_INTERNAL_ERROR,
>>                          _("Not reattaching active device %s"), 
>> dev->name);
>> -        return -1;
>> +        goto exit;
>> +    }
>> +
>> +    virPCIDeviceSetUnbindFromStub(dev, true);
>> +    virPCIDeviceSetRemoveSlot(dev, true);
>> +    virPCIDeviceSetReprobe(dev, true);
>
> are moved here ^^^
>
>> +
>> +    /* Wait for device cleanup if it is qemu/kvm */
>> +    if (virPCIDeviceGetStubDriver(dev) == VIR_PCI_STUB_DRIVER_KVM) {
>> +        int retries = 100;
>> +        while (virPCIDeviceWaitForCleanup(dev, "kvm_assigned_device")
>> +               && retries) {
>> +            usleep(100*1000);
>> +            retries--;
>> +        }
>>       }
>
> and the wait is done here. I'm not against it, but maybe you can 
> explain more why do you think this change is needed? Did you run into 
> any troubles? Also, if we set these three attributes unconditionally, 
> what good is there to keep those APIs around? Also, you forgot to 
> update testVirPCIDeviceReattachSingle().

My point for this change is to make it less confusing. Why do we set those
3 attributes in one place but neglect them in other instances? Basically
we're doing a slight different process in 2-3 places without any particular
reason.

Perhaps a better change would be to remove those 3 setX(dev,true) lines
from virHostdevPCINodeDeviceReAttach flat out (removing the APIs if they
are being used just to set stuff to 'true' around the code), then 
introduce the
wait code inside virPCIDeviceReattach().  And also update
testVirPCIDeviceReattachSingle(), of course.



I'll respin this up.



>
>>       if (virPCIDeviceUnbindFromStub(dev) < 0)
>> -        return -1;
>> +        goto exit;
>>         /* Steal the dev from list inactiveDevs */
>>       if (inactiveDevs) {
>> @@ -1529,7 +1549,10 @@ virPCIDeviceReattach(virPCIDevicePtr dev,
>>           virPCIDeviceListDel(inactiveDevs, dev);
>>       }
>>   -    return 0;
>> +    ret = 0;
>> +
>> + exit:
>
> This is a useless label.
>
> Michal