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

[Marc-André Lureau]

Hi

> +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() ?

> +{
> +    if (status) {

any value of status != 0? I realized reading later on that status type
should be a gboolean

> +        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?

> +
> +/*
> + * The callback function to call when usb driver installation
> + * completes ( == upon a reply from usbclerk)
> + */
> +extern void spice_usb_drv_install_finished(
> +     SpiceUsbDeviceManager *manager,
> +     libusb_device *device,
> +     int ack);

It makes sense to have the function declared in gtk/usb-device-manager-priv.h

> +static gboolean get_vid_pid(libusb_device *device, guint16 *pvid, guint16 *ppid)
> +{
> +    struct libusb_device_descriptor devdesc;
> +    gint r;
> +
> +    g_return_val_if_fail(device != NULL && pvid != NULL && ppid != NULL, FALSE);

In general, it's better to split the argument on different lines, so
that the warning allows you to spot the flawed argument.

> +    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);

I'd put the SPICE_DEBUG in the function itself.

> +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);

g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(manager), FALSE)
g_return_val_if_fail(device != NULL, FALSE)

> +    info = malloc(sizeof(*info));

info = g_new0(InstallInfo, 1)

> +    if (!info) {
> +        g_warning("FAILED to allocate memory for requesting driver installation");
> +        goto install_failed;
> +    }

glib/gtk/spice-gtk doesn't handle out-of-memory condition. You can
remove this check.

> +    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

> +    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.

> +gboolean spice_win_usb_rescan_devices(SpiceUsbDeviceManager *manager,
> +                                      libusb_context *ctx)
> +{
> +    libusb_device **dev_list = NULL;
> +
> +    g_return_val_if_fail(manager != NULL && ctx != NULL, FALSE);

g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(manager), FALSE)
g_return_val_if_fail(ctx != NULL, FALSE)

> +
> +    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?

-- 
Marc-André Lureau