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@xxxxxxxxxxxxx>
>> ---
>
> 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@xxxxxxxxx>
>
>
>
>
>
>
>
>
>> 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;
>>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
