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