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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel