Re: [libvirt] [PATCH v2 06/11] qemu: handle race on device deletion and usb host device plugging
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(a)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().
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(a)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;