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

[Alon Levy]

On Tue, May 22, 2012 at 12:29:42AM +0200, Marc-André Lureau wrote:
> 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)

would this work? if so it's better, no explicit type name:
g_new0(*info, 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
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel