[Spice-devel] [spice-gtk Win32 v2 PATCH 4/5] Windows mingw: usb: Dynamically install a libusb driver for USB devices

[Uri Lublin]

Hi Hans,

Thanks for your review and comments.

On 05/22/2012 11:24 AM, Hans de Goede wrote:
> On 05/20/2012 06:34 PM, Uri Lublin wrote:
>> - Added win-usb-driver-install.[ch]
>> - Added usbclerk.h
>>
>>   if OS_WIN32
>> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
>> index 2b6ce28..c373447 100644
>> --- a/gtk/usb-device-manager.c
>> +++ b/gtk/usb-device-manager.c
>> @@ -32,6 +32,7 @@
>>   #include<gudev/gudev.h>
>>   #elif defined(G_OS_WIN32)
>>   #include "win-usb-dev.h"
>> +#include "win-usb-driver-install.h"
>>   #else
>>   #warning "Expecting one of G_OS_WIN32 and USE_GUDEV to be defined"
>>   #endif
>> @@ -593,11 +594,16 @@ static void 
>> spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>>                               priv->auto_conn_filter_rules_count,
>>                               device, 0) == 0;
>>
>> -        if (can_redirect&&  auto_ok)
>> +        if (can_redirect&&  auto_ok) {
>> +#ifdef G_OS_WIN32
>> +            spice_win_usb_driver_install(self, device);
>
> You probably want to ref device here, to ensure that it sticks around
> until spice_usb_drv_install_finished gets called...
ok

>
> Also IMHO it would be a lot cleaner to give spice_win_usb_driver_install
> a callback function to call, rather then having it hardcoded to
> call spice_usb_drv_install_finished.
ok.
It would get the same params -- (manager, device, result).
I saw some glib/spice-gtk interfaces where a _finished() function
is to be called upon completion.

>
>> +#else
>>               spice_usb_device_manager_connect_device_async(self,
>>                                      (SpiceUsbDevice *)device, NULL,
>>                                      
>> spice_usb_device_manager_auto_connect_cb,
>>                                      libusb_ref_device(device));
>> +#endif
>> +        }
>>       }
>>
>>       SPICE_DEBUG("device added %p", device);
>> @@ -661,6 +667,31 @@ static void 
>> spice_usb_device_manager_channel_connect_cb(
>>       g_object_unref(result);
>>   }
>>
>> +#ifdef G_OS_WIN32
>> +/**
>> + * spice_usb_drv_install_finished:
>> + * @self: #SpiceUsbDeviceManager
>> + * @device: the libusb device for which a libusb driver got installed
>> + * @status: status of driver-installation operation
>> + *
>> + * Called when an Windows libusb driver installation completed.
>> + *
>> + * If the driver installation was successful, continue with USB
>> + * device redirection
>> + */
>> +void spice_usb_drv_install_finished(SpiceUsbDeviceManager *self,
>> +     libusb_device *device, int status)
>> +{
>> +    if (status) {
>> +        spice_win_usb_rescan_devices(self, self->priv->context);
>> +        spice_usb_device_manager_connect_device_async(self,
>> +                (SpiceUsbDevice *)device, NULL,
>> +                spice_usb_device_manager_auto_connect_cb,
>> +                libusb_ref_device(device));
>
> You're taking the reference here, but at this point the device has
> possibly already been destroyed.
I'll take it before, as you mentioned above.

>
>
> I also think what we're seeing here explains why the whole rescan is not
> working properly and you need the libusb patch for that. It would 
> probably
> be better to forget about the device completely, so remove it from the
> usb-device-manager's list of devices and only remembering its bus and 
> port nr,
> and then re-add it after the driver install. Because now you're passing
> device-info to spice_usb_device_manager_connect_device based on the 
> device
> state from before the driver installation.

That and possibly the application itself too (e.g. the widget)

>
> ###
>
> I also note that you only handle auto-connect here, you should of course
> also do a driver install for a manual connect, specifically it would 
> be best
> to move the whole driver install to 
> spice_usb_device_manager_connect_device_async
> so that it will work for both the manual and auto-connect codepaths 
> and use
> the same code for that.

That's a good point.

>>
>> +/**
>> + *
>> + * Start libusb driver installation for @device
>> + *
>> + * A new NamedPipe is created for each request.
>> + *
>> + * Returns: TRUE if a request was sent to usbclerk
>> + *          FALSE upon failure to send a request.
>> + */
>> +gboolean spice_win_usb_driver_install(SpiceUsbDeviceManager *manager,
>> +                                      libusb_device *device)
>> +{
>> +    DWORD bytes;
>> +    HANDLE pipe;
>> +    USBClerkDriverOp req;
>> +    guint16 vid, pid;
>> +    InstallInfo  *info = NULL;
>> +
>> +    g_return_val_if_fail(manager != NULL&&  device != NULL, FALSE);
>> +
>> +    if (! get_vid_pid(device,&vid,&pid)) {
>> +        return FALSE;
>> +    }
>> +
>> +    pipe = CreateFile(USB_CLERK_PIPE_NAME, GENERIC_READ | 
>> GENERIC_WRITE,
>> +                      0, NULL, OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 
>> NULL);
>> +    if (pipe == INVALID_HANDLE_VALUE) {
>> +        g_warning("Cannot open pipe %s %ld", USB_CLERK_PIPE_NAME, 
>> GetLastError());
>> +        return FALSE;
>> +    }
>> +
>> +    SPICE_DEBUG("Sending request to install libusb driver for 
>> %04x:%04x",
>> +                vid, pid);
>> +
>> +    info = malloc(sizeof(*info));
>> +    if (!info) {
>> +        g_warning("FAILED to allocate memory for requesting driver 
>> installation");
>> +        goto install_failed;
>> +    }
>> +
>> +    req.hdr.magic   = USB_CLERK_MAGIC;
>> +    req.hdr.version = USB_CLERK_VERSION;
>> +    req.hdr.type    = USB_CLERK_DRIVER_INSTALL;
>> +    req.hdr.size    = sizeof(req);
>> +    req.vid = vid;
>> +    req.pid = pid;
> +
>
> It would be better to do the install based on bus and port numbers, 
> what if the
> user has 2 identical usb devices, and wants to redirect one?

Yes, I agree.
We had a problem to match libusb's bus/addr to Windows USB device 
enumeration.
We are working on doing this.
Note that this is only a problem for two devices with the exact same 
vid/pid.

>
>
>> +
>> +   /* send request to the service */
>> +    if (!WriteFile(pipe,&req, sizeof(req),&bytes, NULL)) {
>> +        g_warning("Failed to write request to pipe err=%lu", 
>> GetLastError());
>> +        goto install_failed;
>> +    }
>> +    SPICE_DEBUG("Sent request of size %ld bytes to the service", 
>> bytes);
>> +
>> +
>> +    memset(info, 0, sizeof(*info));
>> +    info->manager = manager;
>> +    info->device  = device;
>> +    info->pipe    = pipe;
>> +    if (!ReadFileEx(pipe,&info->ack, 
>> sizeof(info->ack),&info->overlapped,
>> +                    handle_usb_service_reply)) {
>> +        g_warning("Failed to read reply from pipe err=%lu", 
>> GetLastError());
>> +        goto install_failed;
>> +    }
>> +
>> +    SPICE_DEBUG("Waiting (async) for a reply from usb service");
>> +
>> +    return TRUE;
>> +
>> + install_failed:
>> +    CloseHandle(pipe);
>> +    free(info);
>> +
>> +    return FALSE;
>> +}
>> +
>>
>
> Maybe a strange question, but why not use glib functions for this ? These
> functions will neatly follow the gio model including cancellation, etc.
>
> Note that cancelling the read from the pipe will of course not actually
> cancel the driver install being performed...
>

I thought of doing that.
I can change this code later to using the gio model.
Since it's such a simple single-write+single-read scenario, I used 
Windows api.

Thanks,
     Uri.