Hi Hans,
Thanks for your review and comments.
On 05/22/2012 11:24 AM, Hans de Goede wrote:
On 05/20/2012 06:34 PM, Uri Lublin wrote:
- Added win-usb-driver-install.[ch]
- Added usbclerk.h
if OS_WIN32
diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
index 2b6ce28..c373447 100644
--- a/gtk/usb-device-manager.c
+++ b/gtk/usb-device-manager.c
@@ -32,6 +32,7 @@
#include<gudev/gudev.h>
#elif defined(G_OS_WIN32)
#include "win-usb-dev.h"
+#include "win-usb-driver-install.h"
#else
#warning "Expecting one of G_OS_WIN32 and USE_GUDEV to be defined"
#endif
@@ -593,11 +594,16 @@ static void
spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
priv->auto_conn_filter_rules_count,
device, 0) == 0;
- if (can_redirect&& auto_ok)
+ if (can_redirect&& auto_ok) {
+#ifdef G_OS_WIN32
+ spice_win_usb_driver_install(self, device);
You probably want to ref device here, to ensure that it sticks around
until spice_usb_drv_install_finished gets called...
ok
Also IMHO it would be a lot cleaner to give spice_win_usb_driver_install
a callback function to call, rather then having it hardcoded to
call spice_usb_drv_install_finished.
ok.
It would get the same params -- (manager, device, result).
I saw some glib/spice-gtk interfaces where a _finished() function
is to be called upon completion.
+#else
spice_usb_device_manager_connect_device_async(self,
(SpiceUsbDevice *)device, NULL,
spice_usb_device_manager_auto_connect_cb,
libusb_ref_device(device));
+#endif
+ }
}
SPICE_DEBUG("device added %p", device);
@@ -661,6 +667,31 @@ static void
spice_usb_device_manager_channel_connect_cb(
g_object_unref(result);
}
+#ifdef G_OS_WIN32
+/**
+ * spice_usb_drv_install_finished:
+ * @self: #SpiceUsbDeviceManager
+ * @device: the libusb device for which a libusb driver got installed
+ * @status: status of driver-installation operation
+ *
+ * Called when an Windows libusb driver installation completed.
+ *
+ * If the driver installation was successful, continue with USB
+ * device redirection
+ */
+void spice_usb_drv_install_finished(SpiceUsbDeviceManager *self,
+ libusb_device *device, int status)
+{
+ if (status) {
+ 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));
You're taking the reference here, but at this point the device has
possibly already been destroyed.
I'll take it before, as you mentioned above.
I also think what we're seeing here explains why the whole rescan is not
working properly and you need the libusb patch for that. It would
probably
be better to forget about the device completely, so remove it from the
usb-device-manager's list of devices and only remembering its bus and
port nr,
and then re-add it after the driver install. Because now you're passing
device-info to spice_usb_device_manager_connect_device based on the
device
state from before the driver installation.
That and possibly the application itself too (e.g. the widget)
###
I also note that you only handle auto-connect here, you should of course
also do a driver install for a manual connect, specifically it would
be best
to move the whole driver install to
spice_usb_device_manager_connect_device_async
so that it will work for both the manual and auto-connect codepaths
and use
the same code for that.
That's a good point.
+/**
+ *
+ * Start libusb driver installation for @device
+ *
+ * A new NamedPipe is created for each request.
+ *
+ * Returns: TRUE if a request was sent to usbclerk
+ * FALSE upon failure to send a request.
+ */
+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);
+
+ if (! get_vid_pid(device,&vid,&pid)) {
+ return FALSE;
+ }
+
+ pipe = CreateFile(USB_CLERK_PIPE_NAME, GENERIC_READ |
GENERIC_WRITE,
+ 0, NULL, OPEN_EXISTING, FILE_FLAG_OVERLAPPED,
NULL);
+ if (pipe == INVALID_HANDLE_VALUE) {
+ g_warning("Cannot open pipe %s %ld", USB_CLERK_PIPE_NAME,
GetLastError());
+ return FALSE;
+ }
+
+ SPICE_DEBUG("Sending request to install libusb driver for
%04x:%04x",
+ vid, pid);
+
+ info = malloc(sizeof(*info));
+ if (!info) {
+ g_warning("FAILED to allocate memory for requesting driver
installation");
+ goto install_failed;
+ }
+
+ 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;
+
It would be better to do the install based on bus and port numbers,
what if the
user has 2 identical usb devices, and wants to redirect one?
Yes, I agree.
We had a problem to match libusb's bus/addr to Windows USB device
enumeration.
We are working on doing this.
Note that this is only a problem for two devices with the exact same
vid/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);
+
+
+ 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");
+
+ return TRUE;
+
+ install_failed:
+ CloseHandle(pipe);
+ free(info);
+
+ return FALSE;
+}
+
Maybe a strange question, but why not use glib functions for this ? These
functions will neatly follow the gio model including cancellation, etc.
Note that cancelling the read from the pipe will of course not actually
cancel the driver install being performed...
I thought of doing that.
I can change this code later to using the gio model.
Since it's such a simple single-write+single-read scenario, I used
Windows api.
Thanks,
Uri.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel