[Spice-devel,USBCLERK] get_dev_info: ignore devices with no "install_state" information

Submitted by Uri Lublin on June 9, 2013, 11:30 a.m.

Details

Message ID e67b98dfa5bf602948c7d7f82ce8184ef9ffe78c.1370776342.git.uril@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin June 9, 2013, 11:30 a.m.
It may happen (not sure how) that a single device appears
twice in the list of USB devices. Only one entry has information
for the currently installed device/driver.

If that happens, currently we just pick the first one, which
may be an invalid entry that says a nont-WinUSB (e.g. USBSTOR)
is the driver installed, and WinUSB driver removal fails
(saying it's already not installed).

Experiments show that for such an invalid entry, getting "install_state"
property fails.

This patch makes get_dev_info ignore devices with no "install_state"
and pick up the right entry (e.g. with WinUSB driver when driver
removal is requested).
---

I'm not sure why I have not seen this before.
I encountered that problem when running spice-client on a WinXP VM,
and trying to usbredir a USB storage device. Upon stopping usbredir
of the device the WinUSB driver removal by usbclerk failed.
Maybe changes in qemu-kvm USB emulation caused this, maybe
something else.

---
 usbclerk.cpp |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

Patch hide | download patch | download mbox

diff --git a/usbclerk.cpp b/usbclerk.cpp
index 7df11f5..170ff99 100644
--- a/usbclerk.cpp
+++ b/usbclerk.cpp
@@ -616,12 +616,17 @@  bool USBClerk::get_dev_info(HDEVINFO devs, int vid, int pid, SP_DEVINFO_DATA *de
     TCHAR dev_id[MAX_DEVICE_ID_LEN];
     TCHAR service_name[MAX_DEVICE_PROP_LEN];
     bool dev_found = false;
+    DWORD state_install;
 
     _sntprintf(dev_prefix, MAX_DEVICE_ID_LEN, TEXT("USB\\VID_%04X&PID_%04X\\"), vid, pid);
     dev_info->cbSize = sizeof(*dev_info);
     for (DWORD dev_index = 0; SetupDiEnumDeviceInfo(devs, dev_index, dev_info); dev_index++) {
         if (SetupDiGetDeviceInstanceId(devs, dev_info, dev_id, MAX_DEVICE_ID_LEN, NULL) &&
-                (dev_found = !!wcsstr(dev_id, dev_prefix))) {
+            wcsstr(dev_id, dev_prefix) &&
+            SetupDiGetDeviceRegistryProperty(devs, dev_info, SPDRP_INSTALL_STATE, NULL,
+                                             (PBYTE)&state_install, sizeof(state_install),
+                                             NULL)) {
+            dev_found = true;
             break;
         }
     }

Comments

ACK.

On 06/09/2013 01:30 PM, Uri Lublin wrote:
> It may happen (not sure how) that a single device appears
> twice in the list of USB devices. Only one entry has information
> for the currently installed device/driver.
>
> If that happens, currently we just pick the first one, which
> may be an invalid entry that says a nont-WinUSB (e.g. USBSTOR)
> is the driver installed, and WinUSB driver removal fails
> (saying it's already not installed).
>
> Experiments show that for such an invalid entry, getting "install_state"
> property fails.
>
> This patch makes get_dev_info ignore devices with no "install_state"
> and pick up the right entry (e.g. with WinUSB driver when driver
> removal is requested).
> ---
>
> I'm not sure why I have not seen this before.
> I encountered that problem when running spice-client on a WinXP VM,
> and trying to usbredir a USB storage device. Upon stopping usbredir
> of the device the WinUSB driver removal by usbclerk failed.
> Maybe changes in qemu-kvm USB emulation caused this, maybe
> something else.
>
> ---
>   usbclerk.cpp |    7 ++++++-
>   1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/usbclerk.cpp b/usbclerk.cpp
> index 7df11f5..170ff99 100644
> --- a/usbclerk.cpp
> +++ b/usbclerk.cpp
> @@ -616,12 +616,17 @@ bool USBClerk::get_dev_info(HDEVINFO devs, int vid, int pid, SP_DEVINFO_DATA *de
>       TCHAR dev_id[MAX_DEVICE_ID_LEN];
>       TCHAR service_name[MAX_DEVICE_PROP_LEN];
>       bool dev_found = false;
> +    DWORD state_install;
>
>       _sntprintf(dev_prefix, MAX_DEVICE_ID_LEN, TEXT("USB\\VID_%04X&PID_%04X\\"), vid, pid);
>       dev_info->cbSize = sizeof(*dev_info);
>       for (DWORD dev_index = 0; SetupDiEnumDeviceInfo(devs, dev_index, dev_info); dev_index++) {
>           if (SetupDiGetDeviceInstanceId(devs, dev_info, dev_id, MAX_DEVICE_ID_LEN, NULL) &&
> -                (dev_found = !!wcsstr(dev_id, dev_prefix))) {
> +            wcsstr(dev_id, dev_prefix) &&
> +            SetupDiGetDeviceRegistryProperty(devs, dev_info, SPDRP_INSTALL_STATE, NULL,
> +                                             (PBYTE)&state_install, sizeof(state_install),
> +                                             NULL)) {
> +            dev_found = true;
>               break;
>           }
>       }
>