[pve-devel] applied-series: [PATCH v4 0/4] Fix #2343 Spice USB3 support

[Thomas Lamprecht]

On 9/23/19 4:23 PM, Aaron Lauterer wrote:
> I did some short tests with the latest commits and there is the following problem:
> 
> Adding a USB3 device or port will result in such an error msg:
> --------
> Parameter verification failed. (400)
> 
> usb1: invalid format - duplicate key in comma-separated list property: usb3
> --------
> 
> The parameter send in the API request is:
> --------
> usb1: host=090c:1000,usb3=1,usb3=1
> --------
> 
> Without looking into the code it looks to me that when selecting a USB3 capable device to pass through, the checkbox is checked but the `usb3=1` parameter is handled by the dropdown components / callback functions.
> 
> Now with the latest changes the checkbox is not disabled anymore in this situation and thus it's value added to the API request.
> 
> When I uncheck the USB3 checkbox after selecting a device / port I can save the changes and `usb3=1` is set.

Thanks for testing this. Fixed by removing the use of getUSBValue() here as
we know also allow to pass a USB3 device through as USB2, so the speed
detection in the method may not be correct anymore.

While previously this was handled by the "if (!/usb3/.test(val) ..."
check, Dominik thinks that it's only there because when he implemented this,
USB3 devices were required to be passed-through as USB3.
I re-tested passing a USB3 through as USB2 and it worked here, so I think
we really can remove this constrain.

> 
> Regards,
> Aaron
> 
> 
> On 9/23/19 8:44 AM, Thomas Lamprecht wrote:
>> On 9/23/19 7:51 AM, Thomas Lamprecht wrote:
>>> On 9/11/19 2:43 PM, Aaron Lauterer wrote:
>>>> This patch series enables USB3 for the passthrough / redirection of USB
>>>> devices via the Spice client.
>>>>
>>>> v3 -> v4:
>>>> * cleanup of redundant if condition
>>>> * avoiding cyclic module dependency from USB.pm to QemuServer.pm
>>>> * fixing regex check for migration to match if spice is at beginning
>>>>    alone or with a following comma. Thanks Thomas for pointing out how to
>>>>    make it future proof
>>>>
>>>
>>>
>>> applied, thanks!
>>>
>>> I added also cfg2cmd test for this, as we have a "fake QEMU version"
>>> mechanism there I added one with 4.1 as version to see the command effects
>>> and ensuring it keeps stable.
>>>
>>
>> had to do some late-followups:
>>
>> The usb3 format description was now wrong, so:
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 33bf966..ad6902f 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -1319,7 +1319,7 @@ EODESCR
>>       usb3 => {
>>          optional => 1,
>>          type => 'boolean',
>> -       description => "Specifies whether if given host option is a USB3 device or port (this does currently not work reliably with spice redirection and is then ignored).",
>> +       description => "Specifies whether if given host option is a USB3 device or port.",
>>           default => 0,
>>       },
>>   };
>>
>>
>> Further, the "USB 3" disabling in the web interface made no sense anymore,
>> I removed it and refactored the whole component slightly (check out the
>> followup commits for details)
>>
>>>
>>>> v2 -> v3:
>>>> * don't modify current behavior
>>>> * fix local resource check
>>>> * fix and cleanup GUI code
>>>>
>>>> v1 -> v2:
>>>> * no qemu version checks
>>>> * fix local resource check on migration
>>>> * add GUI support
>>>>
>>>> Aaron Lauterer (4):
>>>>
>>>> qemu-server:
>>>>    usb: Cleanup redundant if condition
>>>>    usb: Add USB3 capabilities to Spice USB devices
>>>>    usb: Fix local resource check of Spice USB devices
>>>>
>>>>   PVE/QemuServer.pm     |  7 +++++--
>>>>   PVE/QemuServer/USB.pm | 11 +++++++----
>>>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> pve-manager:
>>>>    usb: Enable USB3 for Spice USB passthrough
>>>>
>>>>   www/manager6/qemu/USBEdit.js | 11 ++++++-----
>>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>