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

[Uri Lublin]

On 05/22/2012 01:29 AM, Marc-André Lureau wrote:

Hi Marc-Andre,

Thanks for the review.
I'll fix all your comments that are not mentioned below.

>> +void spice_usb_drv_install_finished(SpiceUsbDeviceManager *self,
>> +     libusb_device *device, int status)
> nitpick, It's a bit better to keep same prefix for function name:
> perhaps spice_usb_device_manager_win_device_connect() ?
ok, I'll rename it to spice_usb_device_manager_win_drv_install_finished.

>> +{
>> +    if (status) {
> any value of status != 0? I realized reading later on that status type
> should be a gboolean

Currently status is only 1 or 0 (success/failure).
In the future it can hold more values (e.g. driver-is-already-installed).

>> +        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));
>> +    }
> What if status == 0 ? No logging/debug here at least?

There is logging in the calling function.
I'll add a log message here too.


>> +    SPICE_DEBUG("Calling usb-device-manager to continue with usb redir");
>> +    spice_usb_drv_install_finished(manager, device, ack);
> So perhaps simply:
> if (success)
>   spice_usb_device_manager_win_device_connect(manager, device);
Currently that would work, but if in the future we'll have more return 
values
we'll have to add the third parameter.

> I'd put the SPICE_DEBUG in the function itself.
ok

>> +    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;
>> +
>> +   /* 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);
> It's missing a check for completion perhaps:
> if (bytes != sizeof(req))
>    goto install_failed

I'll fix that.

>> +    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");
> What if the reply doesn't come back? How can we cancel the request?
> How do we prevent multiple concurrent request to the same device?
>
> I think we need to do that, because the session and usb-device-manager
> might be finalized before a reply come back for instance.

I assume that sometime in the near future a reply would come back.
That reply can be an error too (e.g. if the win-usb-clerk dies).

I can implement a fucntion to cancel that call, if needed (using 
CancelIoEx).
So should I keep a reference to the usb-device-manager instance, till 
reply comes back ?

We do not prevent multiple concurrent requests.

+
+    SPICE_DEBUG("rescanning libusb devices");
+    libusb_get_device_list(ctx,&dev_list);
+    if (dev_list)
+        libusb_free_device_list(dev_list, 1);


> Why is this needed? There is no event handling for device list changed?

This, together with my libusbx patch, makes libusb recognise the new driver
and start using it in the following libusb_open call (done from 
usbredirhost).