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
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@virtuozzo.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;
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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;
>   

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@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;
>>   
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list