[libvirt] [PATCH v1 2/3] virhostdev: introduce virHostdevReattachAllPCIDevices

[Daniel Henrique Barboza]

On 7/19/19 8:52 AM, Michal Privoznik wrote:
> On 7/18/19 10:10 PM, Daniel Henrique Barboza wrote:
>> There are two places in virhostdev that executes a re-attach operation
>> in all pci devices of a virPCIDeviceListPtr array:
>> virHostdevPreparePCIDevices and virHostdevReAttachPCIDevices. The
>> difference is that the code inside virHostdevPreparePCIDevices uses
>> virPCIDeviceReattach(), while inside virHostdevReAttachPCIDevices
>> virHostdevReattachPCIDevice() is used. Both are called in the
>> same manner inside a loop.
>>
>> To make the code easier to follow and to standardize it further,
>> a new virHostdevReattachAllPCIDevices() helper function is created,
>> which is now called in both places. virHostdevReattachPCIDevice()
>> was chosen as re-attach function because it is an improved version
>> of the raw virPCIDeviceReattach(), including a timer to wait for
>> device cleanup in case of pci_stub_kvm.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   src/util/virhostdev.c | 114 +++++++++++++++++++-----------------------
>>   1 file changed, 52 insertions(+), 62 deletions(-)
>>
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index 7cb0beb545..53aacb59b4 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
>> @@ -613,6 +613,56 @@ 
>> 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();
>> +    }
>> +}
>
> What good is there in introducing this new function if you're removing 
> in the very next patch?

Yep, I'll probably merge this up with patch 3 in a re-spin.



>
>> +
>> +static void
>> +virHostdevReattachAllPCIDevices(virPCIDeviceListPtr pcidevs,
>> +                                virHostdevManagerPtr mgr)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
>> +        virPCIDevicePtr pci = virPCIDeviceListGet(pcidevs, i);
>> +        virPCIDevicePtr actual;
>> +
>> +        /* We need to look up the actual device because that's what
>> +         * virHostdevReattachPCIDevice() expects as its argument */
>> +        if (!(actual = 
>> virPCIDeviceListFind(mgr->inactivePCIHostdevs, pci)))
>> +            continue;
>> +
>> +        if (virPCIDeviceGetManaged(actual))
>> +            virHostdevReattachPCIDevice(mgr, actual);
>
> Just move its internals here. I mean, even from the code you're moving 
> we would be calling virPCIDeviceReattach().
>
>> +        else
>> +            VIR_DEBUG("Not reattaching unmanaged PCI device %s",
>> +                      virPCIDeviceGetName(actual));
>> +    }
>> +}
>> +
>
> Michal