[libvirt] [PATCH v2 06/11] qemu: handle race on device deletion and usb host device plugging

[Nikolay Shirokovskiy]

On 12.09.2019 23:37, Daniel Henrique Barboza wrote:
> 
> 
> On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:
>> Imagine host usb device is unplugged from host and as a result we send
>> command to qemu to delete appropriate device. Then before qemu device is
>> deleted host usb device is plugged back. Currenly code supposes there is
> 
> s/Currenly/Currently
> 
> 
>> no remnant device in qemu and will try to add new device and the attempt
>> will fail.
>>
>> Instead let's check the device is not yet deleted and postpone adding
>> qemu device to device_deleted event handler.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
> 
> Honestly, I tried to give a NACK to this patch, not because of coding
> issues, but because I find it quite convoluted what you're doing here.
> 
> qemuCheckHostdevPlugged() is calling qemuDomainAttachHostDevice()
> if the USB device is found. Problem is that you're calling this check function
> it right after qemuDomainRemoveDevice(), inside a function that is supposed
> to handle delete events. The result is a flow like this:
> 
> - inside processDeviceDeletedEvent:
> 
>     virDomainDefFindDevice(vm->def, devAlias, &dev, true)
> 
>     qemuDomainRemoveDevice(driver, vm, &dev)
> 
>     qemuCheckHostdevPlugged(driver, vm, devAlias);
> 
> - inside qemuCheckHostdevPlugged:
> 
>   virDomainDefFindDevice(vm->def, devAlias, &dev, false) <---- same find you just did
> 
> ( .... other code that verifies if the USB device exists in the host ...)
> 
>   qemuDomainAttachHostDevice(driver, vm, hostdev)
> 
> 
> So in short, you are executing a find(), then a remove(), then the same
> find() again, some code to assert that the USB is plugged in the host,
> then attach().

Yeah this is because remove() in case of unplugging does not actually remove
device from libvirt config, so both find()s find same device.

Nikolay

> 
> Problem is that I didn't come up with a cleaner solution for the problem you're
> solving here, at least considering the changes from the previous patches
> and the current code base. That said:
> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> 
> 
> 
> 
> 
> 
> 
> 
>>   src/qemu/qemu_driver.c | 49 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index f1802b5d44..21640e49c7 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4671,6 +4671,44 @@ processGuestPanicEvent(virQEMUDriverPtr driver,
>>   }
>>     +static int
>> +qemuCheckHostdevPlugged(virQEMUDriverPtr driver,
>> +                        virDomainObjPtr vm,
>> +                        const char *devAlias)
>> +{
>> +    virDomainHostdevDefPtr hostdev;
>> +    virDomainHostdevSubsysUSBPtr usbsrc;
>> +    virDomainDeviceDef dev;
>> +    int num;
>> +
>> +    if (virDomainDefFindDevice(vm->def, devAlias, &dev, false) < 0)
>> +        return 0;
>> +
>> +    if (dev.type != VIR_DOMAIN_DEVICE_HOSTDEV)
>> +        return 0;
>> +
>> +    hostdev = dev.data.hostdev;
>> +    if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
>> +        return 0;
>> +
>> +    usbsrc = &hostdev->source.subsys.u.usb;
>> +    if (!usbsrc->vendor || !usbsrc->product)
>> +        return 0;
>> +
>> +    if ((num = virUSBDeviceFindByVendor(usbsrc->vendor, usbsrc->product,
>> +                                        NULL, false, NULL)) < 0)
>> +        return -1;
>> +
>> +    if (num == 0)
>> +        return 0;
>> +
>> +    if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   static void
>>   processDeviceDeletedEvent(virQEMUDriverPtr driver,
>>                             virDomainObjPtr vm,
>> @@ -4698,6 +4736,11 @@ processDeviceDeletedEvent(virQEMUDriverPtr driver,
>>             if (qemuDomainRemoveDevice(driver, vm, &dev) < 0)
>>               goto endjob;
>> +
>> +        /* Fall thru and save status file even on error condition because
>> +         * device is removed successfully and changed configuration need
>> +         * to be saved in status file. */
>> +        qemuCheckHostdevPlugged(driver, vm, devAlias);
>>       }
>>         if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
>> @@ -5300,6 +5343,12 @@ processUSBAddedEvent(virQEMUDriverPtr driver,
>>       if (i == vm->def->nhostdevs)
>>           goto cleanup;
>>   +    /* if device is not yet even deleted from qemu then handle plugging later.
>> +     * Or we failed handling host usb device unplugging, then another attempt of
>> +     * unplug/plug could help. */
>> +    if (usbsrc->bus || usbsrc->device)
>> +        goto cleanup;
>> +
>>       if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
>>           goto cleanup;
>>   
>