[Spice-devel,spice-gtk,Win32,v4,06/17] Windows mingw: usb: Dynamically install a libusb driver for USB devices

Submitted by Uri Lublin on July 5, 2012, 8:43 p.m.

Details

Message ID 1341521049-4562-7-git-send-email-uril@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin July 5, 2012, 8:43 p.m.
- Added win-usb-driver-install.[ch]
- Added win-usb-clerk.h

Operation (on Windows, spice-gtk point of view):
- After some sanity checks, just before redir'ing a USB device
  a libusb driver needs to be installed (before libusb can open the device)
- A connection (NamedPipe) is established with usb-clerk, a libusb
  driver installation service, and a request for driver installation
  is sent.
- Installation status is asynchronously read from the pipe, and
  spice_usb_drv_install_finished() is called.
- Upon a successful intallation, usbredir continues.

Linux operation is not changed.
---
 gtk/Makefile.am              |    3 +
 gtk/usb-device-manager.c     |  150 +++++++++++++++-
 gtk/win-usb-clerk.h          |   35 ++++
 gtk/win-usb-driver-install.c |  387 ++++++++++++++++++++++++++++++++++++++++++
 gtk/win-usb-driver-install.h |   98 +++++++++++
 5 files changed, 663 insertions(+), 10 deletions(-)
 create mode 100644 gtk/win-usb-clerk.h
 create mode 100644 gtk/win-usb-driver-install.c
 create mode 100644 gtk/win-usb-driver-install.h

Patch hide | download patch | download mbox

diff --git a/gtk/Makefile.am b/gtk/Makefile.am
index c3e820b..66aa42d 100644
--- a/gtk/Makefile.am
+++ b/gtk/Makefile.am
@@ -312,6 +312,9 @@  endif
 WIN_USB_FILES= \
 	win-usb-dev.h			\
 	win-usb-dev.c			\
+	win-usb-clerk.h			\
+	win-usb-driver-install.h	\
+	win-usb-driver-install.c	\
 	$(NULL)

 if OS_WIN32
diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
index 0f6ef34..7d33511 100644
--- a/gtk/usb-device-manager.c
+++ b/gtk/usb-device-manager.c
@@ -33,6 +33,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
@@ -144,6 +145,13 @@  static libusb_device *
 spice_usb_device_manager_device_to_libdev(SpiceUsbDeviceManager *self,
                                           SpiceUsbDevice *device);

+static void
+_spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
+                                               SpiceUsbDevice *device,
+                                               GCancellable *cancellable,
+                                               GAsyncReadyCallback callback,
+                                               gpointer user_data);
+
 G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
                     (GBoxedCopyFunc)spice_usb_device_ref,
                     (GBoxedFreeFunc)spice_usb_device_unref)
@@ -154,6 +162,12 @@  G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device, g_object_ref, g_object_unr

 static void spice_usb_device_manager_initable_iface_init(GInitableIface *iface);

+#ifdef G_OS_WIN32
+static void spice_usb_device_manager_drv_install_cb(GObject *gobject,
+                                                    GAsyncResult *res,
+                                                    gpointer user_data);
+#endif
+
 static guint signals[LAST_SIGNAL] = { 0, };

 G_DEFINE_TYPE_WITH_CODE(SpiceUsbDeviceManager, spice_usb_device_manager, G_TYPE_OBJECT,
@@ -635,7 +649,7 @@  static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
             spice_usb_device_manager_connect_device_async(self,
                                    device, NULL,
                                    spice_usb_device_manager_auto_connect_cb,
-                                   g_object_ref(device));
+                                   device);
         }
     }

@@ -700,6 +714,87 @@  static void spice_usb_device_manager_channel_connect_cb(
     g_object_unref(result);
 }

+#ifdef G_OS_WIN32
+
+typedef struct _UsbInstallCbInfo {
+    SpiceUsbDeviceManager *manager;
+    SpiceUsbDevice        *device;
+    SpiceWinUsbDriver     *installer;
+    GCancellable          *cancellable;
+    GAsyncReadyCallback   callback;
+    gpointer              user_data;
+} UsbInstallCbInfo;
+
+/**
+ * spice_usb_device_manager_drv_install_cb:
+ * @gobject: #SpiceWinUsbDriver in charge of installing the driver
+ * @res: #GAsyncResult of async win usb driver installation
+ * @user_data: #SpiceUsbDeviceManager requested the installation
+ *
+ * Called when an Windows libusb driver installation completed.
+ *
+ * If the driver installation was successful, continue with USB
+ * device redirection
+ */
+static void spice_usb_device_manager_drv_install_cb(GObject *gobject,
+                                                    GAsyncResult *res,
+                                                    gpointer user_data)
+{
+    SpiceUsbDeviceManager *self;
+    SpiceWinUsbDriver *installer;
+    gint status;
+    GError *err = NULL;
+    SpiceUsbDevice *device;
+    UsbInstallCbInfo *cbinfo;
+    GCancellable *cancellable;
+    GAsyncReadyCallback callback;
+
+    SPICE_DEBUG("Win USB driver Installation finished");
+
+    g_return_if_fail(user_data != NULL);
+
+    cbinfo = user_data;
+    self        = cbinfo->manager;
+    device      = cbinfo->device;
+    installer   = cbinfo->installer;
+    cancellable = cbinfo->cancellable;
+    callback    = cbinfo->callback;
+    user_data   = cbinfo->user_data;
+
+    g_free(cbinfo);
+
+    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
+    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(installer));
+    g_return_if_fail(device!= NULL);
+
+    status = spice_win_usb_driver_install_finish(installer, res, &err);
+
+    g_object_unref(installer);
+
+    if (err) {
+        g_warning("win usb driver installation failed -- %s",
+                  err->message);
+        g_error_free(err);
+        spice_usb_device_unref(device);
+        return;
+    }
+
+    if (!status) {
+        g_warning("failed to install win usb driver (status=0)");
+        spice_usb_device_unref(device);
+        return;
+    }
+
+    /* device is already ref'ed */
+    _spice_usb_device_manager_connect_device_async(self,
+                                                   device,
+                                                   cancellable,
+                                                   callback,
+                                                   user_data);
+
+}
+#endif
+
 /* ------------------------------------------------------------------ */
 /* private api                                                        */

@@ -873,6 +968,7 @@  gboolean spice_usb_device_manager_is_device_connected(SpiceUsbDeviceManager *sel
     return !!spice_usb_device_manager_get_channel_for_dev(self, device);
 }

+#ifdef USE_USBREDIR
 /**
  * spice_usb_device_manager_connect_device_async:
  * @manager: the #SpiceUsbDeviceManager manager
@@ -881,11 +977,12 @@  gboolean spice_usb_device_manager_is_device_connected(SpiceUsbDeviceManager *sel
  * @callback: a #GAsyncReadyCallback to call when the request is satisfied
  * @user_data: data to pass to callback
  */
-void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
-                                             SpiceUsbDevice *device,
-                                             GCancellable *cancellable,
-                                             GAsyncReadyCallback callback,
-                                             gpointer user_data)
+static void
+_spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
+                                               SpiceUsbDevice *device,
+                                               GCancellable *cancellable,
+                                               GAsyncReadyCallback callback,
+                                               gpointer user_data)
 {
     GSimpleAsyncResult *result;

@@ -897,7 +994,6 @@  void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
     result = g_simple_async_result_new(G_OBJECT(self), callback, user_data,
                                spice_usb_device_manager_connect_device_async);

-#ifdef USE_USBREDIR
     SpiceUsbDeviceManagerPrivate *priv = self->priv;
     libusb_device *libdev;
     guint i;
@@ -924,17 +1020,51 @@  void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
         libusb_unref_device(libdev);
         return;
     }
-#endif

     g_simple_async_result_set_error(result,
                             SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
                             _("No free USB channel"));
-#ifdef USE_USBREDIR
 done:
-#endif
     g_simple_async_result_complete_in_idle(result);
     g_object_unref(result);
 }
+#endif
+
+
+void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
+                                             SpiceUsbDevice *device,
+                                             GCancellable *cancellable,
+                                             GAsyncReadyCallback callback,
+                                             gpointer user_data)
+{
+
+#ifdef USE_USBREDIR
+    device = spice_usb_device_ref(device);
+
+#ifdef G_OS_WIN32
+    SpiceWinUsbDriver *installer;
+    UsbInstallCbInfo *cbinfo;
+
+    installer = spice_win_usb_driver_new();
+    cbinfo = g_new0(UsbInstallCbInfo, 1);
+    cbinfo->manager     = self;
+    cbinfo->device      = device;
+    cbinfo->installer   = installer;
+    cbinfo->cancellable = cancellable;
+    cbinfo->callback    = callback;
+    cbinfo->user_data   = user_data;
+    spice_win_usb_driver_install(installer, device, NULL,
+                                 spice_usb_device_manager_drv_install_cb,
+                                 cbinfo);
+#else
+    _spice_usb_device_manager_connect_device_async(self,
+                                                   device,
+                                                   cancellable,
+                                                   callback,
+                                                   user_data);
+#endif
+#endif
+}

 gboolean spice_usb_device_manager_connect_device_finish(
     SpiceUsbDeviceManager *self, GAsyncResult *res, GError **err)
diff --git a/gtk/win-usb-clerk.h b/gtk/win-usb-clerk.h
new file mode 100644
index 0000000..5b1e3cf
--- /dev/null
+++ b/gtk/win-usb-clerk.h
@@ -0,0 +1,35 @@ 
+#ifndef _H_USBCLERK
+#define _H_USBCLERK
+
+#include <windows.h>
+
+#define USB_CLERK_PIPE_NAME     TEXT("\\\\.\\pipe\\usbclerkpipe")
+#define USB_CLERK_MAGIC         0xDADA
+#define USB_CLERK_VERSION       0x0002
+
+typedef struct USBClerkHeader {
+    UINT16 magic;
+    UINT16 version;
+    UINT16 type;
+    UINT16 size;
+} USBClerkHeader;
+
+enum {
+    USB_CLERK_DRIVER_INSTALL = 1,
+    USB_CLERK_DRIVER_REMOVE,
+    USB_CLERK_REPLY,
+    USB_CLERK_END_MESSAGE,
+};
+
+typedef struct USBClerkDriverOp {
+    USBClerkHeader hdr;
+    UINT16 vid;
+    UINT16 pid;
+} USBClerkDriverOp;
+
+typedef struct USBClerkReply {
+    USBClerkHeader hdr;
+    UINT32 status;
+} USBClerkReply;
+
+#endif
diff --git a/gtk/win-usb-driver-install.c b/gtk/win-usb-driver-install.c
new file mode 100644
index 0000000..0ca981b
--- /dev/null
+++ b/gtk/win-usb-driver-install.c
@@ -0,0 +1,387 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011 Red Hat, Inc.
+
+   Red Hat Authors:
+   Uri Lublin <uril@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <windows.h>
+#include <gio/gio.h>
+#include <gio/gwin32inputstream.h>
+#include <gio/gwin32outputstream.h>
+#include "spice-util.h"
+#include "win-usb-clerk.h"
+#include "win-usb-driver-install.h"
+#include "usb-device-manager-priv.h"
+
+/* ------------------------------------------------------------------ */
+/* gobject glue                                                       */
+
+#define SPICE_WIN_USB_DRIVER_GET_PRIVATE(obj)     \
+    (G_TYPE_INSTANCE_GET_PRIVATE ((obj), SPICE_TYPE_WIN_USB_DRIVER, SpiceWinUsbDriverPrivate))
+
+struct _SpiceWinUsbDriverPrivate {
+    USBClerkReply         reply;
+    GSimpleAsyncResult    *result;
+    GCancellable          *cancellable;
+    gulong                cancellable_id;
+    HANDLE                handle;
+    SpiceUsbDevice        *device;
+};
+
+
+
+G_DEFINE_TYPE(SpiceWinUsbDriver, spice_win_usb_driver, G_TYPE_OBJECT);
+
+static void spice_win_usb_driver_init(SpiceWinUsbDriver *self)
+{
+    self->priv = SPICE_WIN_USB_DRIVER_GET_PRIVATE(self);
+}
+
+static void spice_win_usb_driver_close(SpiceWinUsbDriver *self)
+{
+    if (self->priv->handle) {
+        CloseHandle(self->priv->handle);
+        self->priv->handle = 0;
+    }
+}
+
+static void spice_win_usb_driver_cleanup(SpiceWinUsbDriver *self)
+{
+    SpiceWinUsbDriverPrivate *priv = self->priv;
+
+    spice_win_usb_driver_close(self);
+
+    if (priv->cancellable_id) { /* FIXME: should be in acl_helper too ? */
+        g_cancellable_disconnect(priv->cancellable, priv->cancellable_id);
+        priv->cancellable = NULL;
+        priv->cancellable_id = 0;
+    }
+
+    g_clear_object(&priv->result);
+
+}
+
+static void spice_win_usb_driver_finalize(GObject *gobject)
+{
+    spice_win_usb_driver_cleanup(SPICE_WIN_USB_DRIVER(gobject));
+}
+
+static void spice_win_usb_driver_class_init(SpiceWinUsbDriverClass *klass)
+{
+    GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
+
+    gobject_class->finalize     = spice_win_usb_driver_finalize;
+
+    g_type_class_add_private(klass, sizeof(SpiceWinUsbDriverPrivate));
+}
+
+/* ------------------------------------------------------------------ */
+/* callbacks                                                          */
+
+static void win_usb_driver_async_result_set_cancelled(GSimpleAsyncResult *result)
+{
+    g_simple_async_result_set_error(result,
+                G_IO_ERROR, G_IO_ERROR_CANCELLED,
+                "Win USB driver installation cancelled");
+}
+
+static void win_usb_driver_cancelled_cb(GCancellable *cancellable, gpointer user_data)
+{
+    SpiceWinUsbDriver *self = SPICE_WIN_USB_DRIVER(user_data);
+    SpiceWinUsbDriverPrivate *priv = self->priv;
+
+    g_message("IN %s result=%p", __FUNCTION__, priv->result);
+    if (priv->result) {
+        win_usb_driver_async_result_set_cancelled(priv->result);
+        g_simple_async_result_complete_in_idle(priv->result);
+    }
+}
+
+void win_usb_driver_handle_reply_cb(GObject *gobject,
+                                    GAsyncResult *read_res,
+                                    gpointer user_data)
+{
+    SpiceWinUsbDriver *self;
+    SpiceWinUsbDriverPrivate *priv;
+
+    GInputStream *istream;
+    GError *err = NULL;
+    gssize bytes;
+
+    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(user_data));
+    self = SPICE_WIN_USB_DRIVER(user_data);
+    priv = self->priv;
+    istream = (GInputStream *)gobject;
+
+    bytes = g_input_stream_read_finish(istream, read_res, &err);
+
+    SPICE_DEBUG("Finished reading reply-msg from usbclerk: bytes=%ld "
+                "err_exist?=%d", (long)bytes, err!=NULL);
+
+    if (err) {
+        g_warning("failed to read reply from usbclerk (%s)", err->message);
+        g_simple_async_result_take_error(priv->result, err);
+        goto failed_reply;
+    }
+
+    g_warn_if_fail(g_input_stream_close(istream, NULL, NULL));
+    g_clear_object(&istream);
+    spice_win_usb_driver_close(self);
+
+    if (bytes == 0) {
+        g_warning("unexpected EOF from usbclerk");
+        g_simple_async_result_set_error(priv->result,
+                                        SPICE_WIN_USB_DRIVER_ERROR,
+                                        SPICE_WIN_USB_DRIVER_ERROR_FAILED,
+                                        "unexpected EOF from usbclerk");
+        goto failed_reply;
+    }
+
+    if (bytes != sizeof(priv->reply)) {
+        g_warning("usbclerk size mismatch: read %d bytes, expected %d (header %d, size in header %d)",
+                  bytes, sizeof(priv->reply), sizeof(priv->reply.hdr), priv->reply.hdr.size);
+        /* For now just warn, do not fail */
+    }
+
+    if (priv->reply.hdr.magic != USB_CLERK_MAGIC) {
+        g_warning("usbclerk magic mismatch: mine=0x%04x  server=0x%04x",
+                  USB_CLERK_MAGIC, priv->reply.hdr.magic);
+        g_simple_async_result_set_error(priv->result,
+                                        SPICE_WIN_USB_DRIVER_ERROR,
+                                        SPICE_WIN_USB_DRIVER_ERROR_MESSAGE,
+                                        "usbclerk magic mismatch");
+        goto failed_reply;
+    }
+
+    if (priv->reply.hdr.version != USB_CLERK_VERSION) {
+        g_warning("usbclerk version mismatch: mine=0x%04x  server=0x%04x",
+                  USB_CLERK_VERSION, priv->reply.hdr.version);
+        /* For now just warn, do not fail */
+    }
+
+    if (priv->reply.hdr.type != USB_CLERK_REPLY) {
+        g_warning("usbclerk message with unexpected type %d",
+                  priv->reply.hdr.type);
+        g_simple_async_result_set_error(priv->result,
+                                        SPICE_WIN_USB_DRIVER_ERROR,
+                                        SPICE_WIN_USB_DRIVER_ERROR_MESSAGE,
+                                        "usbclerk message with unexpected type");
+        goto failed_reply;
+    }
+
+    if (priv->reply.hdr.size != bytes) {
+        g_warning("usbclerk message size mismatch: read %d bytes  hdr.size=%d",
+                  bytes, priv->reply.hdr.size);
+        g_simple_async_result_set_error(priv->result,
+                                        SPICE_WIN_USB_DRIVER_ERROR,
+                                        SPICE_WIN_USB_DRIVER_ERROR_MESSAGE,
+                                        "usbclerk message with unexpected size");
+        goto failed_reply;
+    }
+
+ failed_reply:
+    g_simple_async_result_complete_in_idle(priv->result);
+    g_clear_object(&priv->result);
+}
+
+/* ------------------------------------------------------------------ */
+/* helper functions                                                   */
+
+static
+gboolean spice_win_usb_driver_send_request(SpiceWinUsbDriver *self, guint16 op,
+                                           guint16 vid, guint16 pid, GError **err)
+{
+    USBClerkDriverOp req;
+    GOutputStream *ostream;
+    SpiceWinUsbDriverPrivate *priv;
+    gsize bytes;
+    gboolean ret;
+
+    SPICE_DEBUG("sending a request to usbclerk service (op=%d vid=0x%04x pid=0x%04x",
+                op, vid, pid);
+
+    g_return_val_if_fail(SPICE_IS_WIN_USB_DRIVER(self), FALSE);
+    priv = self->priv;
+
+    memset(&req, 0, sizeof(req));
+    req.hdr.magic   = USB_CLERK_MAGIC;
+    req.hdr.version = USB_CLERK_VERSION;
+    req.hdr.type    = op;
+    req.hdr.size    = sizeof(req);
+    req.vid = vid;
+    req.pid = pid;
+
+    ostream = g_win32_output_stream_new(priv->handle, FALSE);
+
+    ret = g_output_stream_write_all(ostream, &req, sizeof(req), &bytes, NULL, err);
+    g_warn_if_fail(g_output_stream_close(ostream, NULL, NULL));
+    g_object_unref(ostream);
+    SPICE_DEBUG("write_all request returned %d written bytes %u expecting %u",
+                ret, bytes, sizeof(req));
+    return ret;
+}
+
+static
+void spice_win_usb_driver_read_reply_async(SpiceWinUsbDriver *self)
+{
+    SpiceWinUsbDriverPrivate *priv;
+    GInputStream  *istream;
+
+    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(self));
+    priv = self->priv;
+
+    SPICE_DEBUG("waiting for a reply from usbclerk");
+
+    istream = g_win32_input_stream_new(priv->handle, FALSE);
+
+    g_input_stream_read_async(istream, &priv->reply, sizeof(priv->reply),
+                              G_PRIORITY_DEFAULT, NULL,
+                              win_usb_driver_handle_reply_cb, self);
+}
+
+
+/* ------------------------------------------------------------------ */
+/* private api                                                        */
+
+
+G_GNUC_INTERNAL
+SpiceWinUsbDriver *spice_win_usb_driver_new(void)
+{
+    GObject *obj;
+
+    obj = g_object_new(SPICE_TYPE_WIN_USB_DRIVER, NULL);
+
+    return SPICE_WIN_USB_DRIVER(obj);
+}
+
+/**
+ * spice_win_usb_driver_install:
+ * 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.
+ */
+G_GNUC_INTERNAL
+void spice_win_usb_driver_install(SpiceWinUsbDriver *self,
+                                  SpiceUsbDevice *device,
+                                  GCancellable *cancellable,
+                                  GAsyncReadyCallback callback,
+                                  gpointer user_data)
+{
+    guint16 vid, pid;
+    GError *err = NULL;
+    GSimpleAsyncResult *result;
+    SpiceWinUsbDriverPrivate *priv;
+
+    SPICE_DEBUG("Win usb driver installation started");
+
+    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(self));
+    g_return_if_fail(device != NULL);
+
+    priv = self->priv;
+
+    result = g_simple_async_result_new(G_OBJECT(self), callback, user_data,
+                                       spice_win_usb_driver_install);
+
+    vid = spice_usb_device_get_vid(device);
+    pid = spice_usb_device_get_pid(device);
+
+    SPICE_DEBUG("win-usb-driver-install: connecting to usbclerk named pipe");
+    priv->handle = CreateFile(USB_CLERK_PIPE_NAME,
+                              GENERIC_READ | GENERIC_WRITE,
+                              0, NULL,
+                              OPEN_EXISTING,
+                              FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OVERLAPPED,
+                              NULL);
+    if (priv->handle == INVALID_HANDLE_VALUE) {
+        DWORD errval  = GetLastError();
+        gchar *errstr = g_win32_error_message(errval);
+        g_warning("failed to create a named pipe to usbclerk (%ld) %s",
+                  errval,errstr);
+        g_simple_async_result_set_error(result,
+                  G_IO_ERROR, G_IO_ERROR_FAILED,
+                  "Failed to create named pipe (%ld) %s", errval, errstr);
+        goto failed_request;
+    }
+
+    if (!spice_win_usb_driver_send_request(self, USB_CLERK_DRIVER_INSTALL,
+                                           vid, pid, &err)) {
+        g_warning("failed to send a request to usbclerk %s", err->message);
+        g_simple_async_result_take_error(result, err);
+        goto failed_request;
+    }
+
+    /* set up for async read */
+    priv->result = result;
+    priv->device = device;
+    if (cancellable) {
+        priv->cancellable = cancellable;
+        priv->cancellable_id = g_cancellable_connect(cancellable,
+                                                     G_CALLBACK(win_usb_driver_cancelled_cb),
+                                                     self, NULL);
+    }
+
+    spice_win_usb_driver_read_reply_async(self);
+
+    return;
+
+ failed_request:
+    spice_win_usb_driver_close(self);
+    g_simple_async_result_complete_in_idle(result);
+    g_clear_object(&result);
+}
+
+
+/**
+ * Returns: currently returns 0 (failure) and 1 (success)
+ * possibly later we'll add error-codes
+ */
+G_GNUC_INTERNAL
+gint spice_win_usb_driver_install_finish(SpiceWinUsbDriver *self,
+                                          GAsyncResult *res, GError **err)
+{
+    GSimpleAsyncResult *result = G_SIMPLE_ASYNC_RESULT(res);
+
+    g_return_val_if_fail(SPICE_IS_WIN_USB_DRIVER(self), 0);
+    g_return_val_if_fail(g_simple_async_result_is_valid(res, G_OBJECT(self),
+                                                        spice_win_usb_driver_install),
+                         FALSE);
+    if (g_simple_async_result_propagate_error(result, err))
+        return 0;
+
+    return self->priv->reply.status;
+}
+
+G_GNUC_INTERNAL
+SpiceUsbDevice *spice_win_usb_driver_get_device(SpiceWinUsbDriver *self)
+{
+    g_return_val_if_fail(SPICE_IS_WIN_USB_DRIVER(self), 0);
+
+    return self->priv->device;
+}
+
+GQuark spice_win_usb_driver_error_quark(void)
+{
+    return g_quark_from_static_string("spice-win-usb-driver-error-quark");
+}
diff --git a/gtk/win-usb-driver-install.h b/gtk/win-usb-driver-install.h
new file mode 100644
index 0000000..b0ccf33
--- /dev/null
+++ b/gtk/win-usb-driver-install.h
@@ -0,0 +1,98 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011 Red Hat, Inc.
+
+   Red Hat Authors:
+   Uri Lublin <uril@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef SPICE_WIN_USB_DRIVER_H
+#define SPICE_WIN_USB_DRIVER_H
+
+#include "usb-device-manager.h"
+
+G_BEGIN_DECLS
+
+GQuark win_usb_driver_error_quark(void);
+
+
+#define SPICE_TYPE_WIN_USB_DRIVER      (spice_win_usb_driver_get_type ())
+#define SPICE_WIN_USB_DRIVER(obj)      (G_TYPE_CHECK_INSTANCE_CAST ((obj),    \
+            SPICE_TYPE_WIN_USB_DRIVER, SpiceWinUsbDriver))
+#define SPICE_IS_WIN_USB_DRIVER(obj)   (G_TYPE_CHECK_INSTANCE_TYPE ((obj),    \
+            SPICE_TYPE_WIN_USB_DRIVER))
+#define SPICE_WIN_USB_DRIVER_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass),  \
+            SPICE_TYPE_WIN_USB_DRIVER, SpiceWinUsbDriverClass))
+#define SPICE_IS_WIN_USB_DRIVER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass),\
+            SPICE_TYPE_WIN_USB_DRIVER))
+#define SPICE_WIN_USB_DRIVER_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj),\
+            SPICE_TYPE_WIN_USB_DRIVER, SpiceWinUsbDriverClass))
+
+typedef struct _SpiceWinUsbDriver          SpiceWinUsbDriver;
+typedef struct _SpiceWinUsbDriverClass     SpiceWinUsbDriverClass;
+typedef struct _SpiceWinUsbDriverPrivate   SpiceWinUsbDriverPrivate;
+
+struct _SpiceWinUsbDriver
+{
+    GObject parent;
+
+    /*< private >*/
+    SpiceWinUsbDriverPrivate *priv;
+    /* Do not add fields to this struct */
+};
+
+struct _SpiceWinUsbDriverClass
+{
+    GObjectClass parent_class;
+};
+
+GType spice_win_usb_driver_get_type(void);
+
+SpiceWinUsbDriver *spice_win_usb_driver_new(void);
+
+
+void spice_win_usb_driver_install(SpiceWinUsbDriver *self,
+                                  SpiceUsbDevice *device,
+                                  GCancellable *cancellable,
+                                  GAsyncReadyCallback callback,
+                                  gpointer user_data);
+
+gint spice_win_usb_driver_install_finish(SpiceWinUsbDriver *self,
+                                         GAsyncResult *res, GError **err);
+
+
+SpiceUsbDevice *spice_win_usb_driver_get_device(SpiceWinUsbDriver *self);
+
+#define SPICE_WIN_USB_DRIVER_ERROR spice_win_usb_driver_error_quark()
+
+/**
+ * SpiceWinUsbDriverError:
+ * @SPICE_WIN_USB_DRIVER_ERROR_FAILED: generic error code
+ * @SPICE_WIN_USB_DRIVER_ERROR_MESSAGE: bad message read from clerk
+ *
+ * Error codes returned by spice-client API.
+ */
+typedef enum
+{
+    SPICE_WIN_USB_DRIVER_ERROR_FAILED,
+    SPICE_WIN_USB_DRIVER_ERROR_MESSAGE,
+} SpiceWinUsbDriverError;
+
+GQuark spice_win_usb_driver_error_quark(void);
+
+G_END_DECLS
+
+#endif /* SPICE_WIN_USB_DRIVER_H */

Comments

Hi

some remarks below

On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin <uril@redhat.com> wrote:
> @@ -635,7 +649,7 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>              spice_usb_device_manager_connect_device_async(self,
>                                     device, NULL,
>                                     spice_usb_device_manager_auto_connect_cb,
> -                                   g_object_ref(device));
> +                                   device);

This is a bit worrisome, an async operation should keep a reference on
device, or it must make sure to cancel cleanly when the device is
destroyed.

The spice_usb_device_manager_auto_connect_cb is still doing
spice_usb_device_unref(device), is that normal? see also comment in
03/17

> +    installer = spice_win_usb_driver_new();
> +    cbinfo = g_new0(UsbInstallCbInfo, 1);
> +    cbinfo->manager     = self;
> +    cbinfo->device      = device;
> +    cbinfo->installer   = installer;
> +    cbinfo->cancellable = cancellable;
> +    cbinfo->callback    = callback;
> +    cbinfo->user_data   = user_data;
> +    spice_win_usb_driver_install(installer, device, NULL,
> +                                 spice_usb_device_manager_drv_install_cb,
> +                                 cbinfo);

The same cancellable should be used.


> +static void win_usb_driver_cancelled_cb(GCancellable *cancellable, gpointer user_data)
> +{
> +    SpiceWinUsbDriver *self = SPICE_WIN_USB_DRIVER(user_data);
> +    SpiceWinUsbDriverPrivate *priv = self->priv;
> +
> +    g_message("IN %s result=%p", __FUNCTION__, priv->result);

s/g_message/SPICE_DEBUG

> +    if (priv->result) {
> +        win_usb_driver_async_result_set_cancelled(priv->result);
> +        g_simple_async_result_complete_in_idle(priv->result);
> +    }

I think this isn't really cancelling the operation, it will give the
impression to the caller that it is cancelled, by returning an error.
But there will still be an outstanding/ pending operation that may
cause further errors when calling the clerk etc, the device might be
busy or there might be multiple simulatenous requests etc...

> +    istream = (GInputStream *)gobject;

prefer the safer G_INPUT_STREAM macro over a static cast.

> +
> +    if (bytes != sizeof(priv->reply)) {
> +        g_warning("usbclerk size mismatch: read %d bytes, expected %d (header %d, size in header %d)",
> +                  bytes, sizeof(priv->reply), sizeof(priv->reply.hdr), priv->reply.hdr.size);
> +        /* For now just warn, do not fail */

Although a bit unlikely, this read might be incomplete, so you may
want to re-call read_async with the rest of the buffer in this case
(it seems to happen quite often with the legacy usb)

> +    memset(&req, 0, sizeof(req));

Just a note because it was there a couple of time, a declaration such
as USBClerkDriverOp req = { 0, } is less error-prone and elegant than
calling memset() yourself. No problem!

> +    req.hdr.magic   = USB_CLERK_MAGIC;
> +    req.hdr.version = USB_CLERK_VERSION;
> +    req.hdr.type    = op;
> +    req.hdr.size    = sizeof(req);
> +    req.vid = vid;
> +    req.pid = pid;
> +
> +    ostream = g_win32_output_stream_new(priv->handle, FALSE);
> +
> +    ret = g_output_stream_write_all(ostream, &req, sizeof(req), &bytes, NULL, err);

This is a blocking call, in the main thread, afaict

> +    if (!spice_win_usb_driver_send_request(self, USB_CLERK_DRIVER_INSTALL,
> +                                           vid, pid, &err)) {

So this is blocking op

> +    /* set up for async read */
> +    priv->result = result;

So what happen if there is already an outstanding priv->result?
perhaps it should g_return_if_fail condition at the beginning of this
function?

> +    priv->device = device;

Wouldn't it make sense to ref it since the async operation may
out-live the caller?

> +    if (cancellable) {
> +        priv->cancellable = cancellable;
> +        priv->cancellable_id = g_cancellable_connect(cancellable,
> +                                                     G_CALLBACK(win_usb_driver_cancelled_cb),
> +                                                     self, NULL);
> +    }

This isn't really helping, the signal isn't going to cancel the
operation on a different thread, the cancellable should be passed to
all the async operations.

> +    spice_win_usb_driver_read_reply_async(self);

Here
Hi,

On 07/05/2012 10:43 PM, Uri Lublin wrote:
> - Added win-usb-driver-install.[ch]
> - Added win-usb-clerk.h
>
> Operation (on Windows, spice-gtk point of view):
> - After some sanity checks, just before redir'ing a USB device
>    a libusb driver needs to be installed (before libusb can open the device)
> - A connection (NamedPipe) is established with usb-clerk, a libusb
>    driver installation service, and a request for driver installation
>    is sent.
> - Installation status is asynchronously read from the pipe, and
>    spice_usb_drv_install_finished() is called.
> - Upon a successful intallation, usbredir continues.
>
> Linux operation is not changed.
> ---
>   gtk/Makefile.am              |    3 +
>   gtk/usb-device-manager.c     |  150 +++++++++++++++-
>   gtk/win-usb-clerk.h          |   35 ++++
>   gtk/win-usb-driver-install.c |  387 ++++++++++++++++++++++++++++++++++++++++++
>   gtk/win-usb-driver-install.h |   98 +++++++++++
>   5 files changed, 663 insertions(+), 10 deletions(-)
>   create mode 100644 gtk/win-usb-clerk.h
>   create mode 100644 gtk/win-usb-driver-install.c
>   create mode 100644 gtk/win-usb-driver-install.h
>

<snip>

> @@ -635,7 +649,7 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>               spice_usb_device_manager_connect_device_async(self,
>                                      device, NULL,
>                                      spice_usb_device_manager_auto_connect_cb,
> -                                   g_object_ref(device));
> +                                   device);
>           }
>       }
>


This seems wrong as spice_usb_device_manager_auto_connect_cb will still do a spice_usb_device_unref, so
we must ref it here to match, also this seems like something which belongs in a different patch...

> @@ -700,6 +714,87 @@ static void spice_usb_device_manager_channel_connect_cb(
>       g_object_unref(result);
>   }
>
> +#ifdef G_OS_WIN32
> +
> +typedef struct _UsbInstallCbInfo {
> +    SpiceUsbDeviceManager *manager;
> +    SpiceUsbDevice        *device;
> +    SpiceWinUsbDriver     *installer;
> +    GCancellable          *cancellable;
> +    GAsyncReadyCallback   callback;
> +    gpointer              user_data;
> +} UsbInstallCbInfo;
> +
> +/**
> + * spice_usb_device_manager_drv_install_cb:
> + * @gobject: #SpiceWinUsbDriver in charge of installing the driver
> + * @res: #GAsyncResult of async win usb driver installation
> + * @user_data: #SpiceUsbDeviceManager requested the installation
> + *
> + * Called when an Windows libusb driver installation completed.
> + *
> + * If the driver installation was successful, continue with USB
> + * device redirection
> + */
> +static void spice_usb_device_manager_drv_install_cb(GObject *gobject,
> +                                                    GAsyncResult *res,
> +                                                    gpointer user_data)
> +{
> +    SpiceUsbDeviceManager *self;
> +    SpiceWinUsbDriver *installer;
> +    gint status;
> +    GError *err = NULL;
> +    SpiceUsbDevice *device;
> +    UsbInstallCbInfo *cbinfo;
> +    GCancellable *cancellable;
> +    GAsyncReadyCallback callback;
> +
> +    SPICE_DEBUG("Win USB driver Installation finished");
> +
> +    g_return_if_fail(user_data != NULL);
> +
> +    cbinfo = user_data;
> +    self        = cbinfo->manager;
> +    device      = cbinfo->device;
> +    installer   = cbinfo->installer;
> +    cancellable = cbinfo->cancellable;
> +    callback    = cbinfo->callback;
> +    user_data   = cbinfo->user_data;
> +
> +    g_free(cbinfo);
> +
> +    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
> +    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(installer));
> +    g_return_if_fail(device!= NULL);
> +
> +    status = spice_win_usb_driver_install_finish(installer, res, &err);
> +
> +    g_object_unref(installer);
> +
> +    if (err) {
> +        g_warning("win usb driver installation failed -- %s",
> +                  err->message);
> +        g_error_free(err);
> +        spice_usb_device_unref(device);
> +        return;
> +    }
> +
> +    if (!status) {
> +        g_warning("failed to install win usb driver (status=0)");
> +        spice_usb_device_unref(device);

Can you please raise the appropriate error signal here, so that the user gets
an error dialog ?

> +        return;
> +    }
> +
> +    /* device is already ref'ed */
> +    _spice_usb_device_manager_connect_device_async(self,
> +                                                   device,
> +                                                   cancellable,
> +                                                   callback,
> +                                                   user_data);
> +
> +}
> +#endif
> +
>   /* ------------------------------------------------------------------ */
>   /* private api                                                        */
>
> @@ -873,6 +968,7 @@ gboolean spice_usb_device_manager_is_device_connected(SpiceUsbDeviceManager *sel
>       return !!spice_usb_device_manager_get_channel_for_dev(self, device);
>   }
>
> +#ifdef USE_USBREDIR
>   /**
>    * spice_usb_device_manager_connect_device_async:
>    * @manager: the #SpiceUsbDeviceManager manager
> @@ -881,11 +977,12 @@ gboolean spice_usb_device_manager_is_device_connected(SpiceUsbDeviceManager *sel
>    * @callback: a #GAsyncReadyCallback to call when the request is satisfied
>    * @user_data: data to pass to callback
>    */
> -void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
> -                                             SpiceUsbDevice *device,
> -                                             GCancellable *cancellable,
> -                                             GAsyncReadyCallback callback,
> -                                             gpointer user_data)
> +static void
> +_spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
> +                                               SpiceUsbDevice *device,
> +                                               GCancellable *cancellable,
> +                                               GAsyncReadyCallback callback,
> +                                               gpointer user_data)
>   {
>       GSimpleAsyncResult *result;
>
> @@ -897,7 +994,6 @@ void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>       result = g_simple_async_result_new(G_OBJECT(self), callback, user_data,
>                                  spice_usb_device_manager_connect_device_async);
>
> -#ifdef USE_USBREDIR
>       SpiceUsbDeviceManagerPrivate *priv = self->priv;
>       libusb_device *libdev;
>       guint i;
> @@ -924,17 +1020,51 @@ void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>           libusb_unref_device(libdev);
>           return;
>       }
> -#endif
>
>       g_simple_async_result_set_error(result,
>                               SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>                               _("No free USB channel"));
> -#ifdef USE_USBREDIR
>   done:
> -#endif
>       g_simple_async_result_complete_in_idle(result);
>       g_object_unref(result);
>   }
> +#endif
> +
> +
> +void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
> +                                             SpiceUsbDevice *device,
> +                                             GCancellable *cancellable,
> +                                             GAsyncReadyCallback callback,
> +                                             gpointer user_data)
> +{
> +
> +#ifdef USE_USBREDIR
> +    device = spice_usb_device_ref(device);
> +
> +#ifdef G_OS_WIN32
> +    SpiceWinUsbDriver *installer;
> +    UsbInstallCbInfo *cbinfo;
> +
> +    installer = spice_win_usb_driver_new();
> +    cbinfo = g_new0(UsbInstallCbInfo, 1);
> +    cbinfo->manager     = self;
> +    cbinfo->device      = device;
> +    cbinfo->installer   = installer;
> +    cbinfo->cancellable = cancellable;
> +    cbinfo->callback    = callback;
> +    cbinfo->user_data   = user_data;
> +    spice_win_usb_driver_install(installer, device, NULL,
> +                                 spice_usb_device_manager_drv_install_cb,
> +                                 cbinfo);
> +#else
> +    _spice_usb_device_manager_connect_device_async(self,
> +                                                   device,
> +                                                   cancellable,
> +                                                   callback,
> +                                                   user_data);
> +#endif
> +#endif
> +}
>
>   gboolean spice_usb_device_manager_connect_device_finish(
>       SpiceUsbDeviceManager *self, GAsyncResult *res, GError **err)

<snip>

Regards,

Hans
On 07/06/2012 01:28 AM, Marc-André Lureau wrote:
> some remarks below
>
> On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin<uril@redhat.com>  wrote:
>> @@ -635,7 +649,7 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>>               spice_usb_device_manager_connect_device_async(self,
>>                                      device, NULL,
>>                                      spice_usb_device_manager_auto_connect_cb,
>> -                                   g_object_ref(device));
>> +                                   device);
> This is a bit worrisome, an async operation should keep a reference on
> device, or it must make sure to cancel cleanly when the device is
> destroyed.
>
> The spice_usb_device_manager_auto_connect_cb is still doing
> spice_usb_device_unref(device), is that normal? see also comment in
> 03/17

In V4, I moved it (reference incremental) to 
spice_usb_device_manager_connect_device_async.

In V5, I keep it here, where it's correct.

>> +static void win_usb_driver_cancelled_cb(GCancellable *cancellable, gpointer user_data)
>> +{
>> +    SpiceWinUsbDriver *self = SPICE_WIN_USB_DRIVER(user_data);
>> +    SpiceWinUsbDriverPrivate *priv = self->priv;
>> +
>> +    g_message("IN %s result=%p", __FUNCTION__, priv->result);
> s/g_message/SPICE_DEBUG

Since I expect this operation to not occur many times, I used g_message.
I removed that code in V5.

>
>> +    if (priv->result) {
>> +        win_usb_driver_async_result_set_cancelled(priv->result);
>> +        g_simple_async_result_complete_in_idle(priv->result);
>> +    }
> I think this isn't really cancelling the operation, it will give the
> impression to the caller that it is cancelled, by returning an error.
> But there will still be an outstanding/ pending operation that may
> cause further errors when calling the clerk etc, the device might be
> busy or there might be multiple simulatenous requests etc...

ok. In V5 the installer does not register for cancellation events.
It just pass the cancellable to ginputstream and hopefully that's enough.


>
>> +
>> +    if (bytes != sizeof(priv->reply)) {
>> +        g_warning("usbclerk size mismatch: read %d bytes, expected %d (header %d, size in header %d)",
>> +                  bytes, sizeof(priv->reply), sizeof(priv->reply.hdr), priv->reply.hdr.size);
>> +        /* For now just warn, do not fail */
> Although a bit unlikely, this read might be incomplete, so you may
> want to re-call read_async with the rest of the buffer in this case
> (it seems to happen quite often with the legacy usb)

I did not fix it in V5.
Also I did not experience it.
Something for the future.

>
>> +    memset(&req, 0, sizeof(req));
> Just a note because it was there a couple of time, a declaration such
> as USBClerkDriverOp req = { 0, } is less error-prone and elegant than
> calling memset() yourself. No problem!

I'm not sure {0, } is less error-prone.
For example for static variables it can be confusing.
In V5, I modified it anyway.

>> +
>> +    ostream = g_win32_output_stream_new(priv->handle, FALSE);
>> +
>> +    ret = g_output_stream_write_all(ostream,&req, sizeof(req),&bytes, NULL, err);
> This is a blocking call, in the main thread, afaict

Yes. It's the first write on this named-pipe, and a small one.
I expect it to not block.
If that's a problem, I'll change it to an async write.

>> +    /* set up for async read */
>> +    priv->result = result;
> So what happen if there is already an outstanding priv->result?
> perhaps it should g_return_if_fail condition at the beginning of this
> function?
ok. I added a g_return_if_fail on priv->result.
Every installer opens its own pipe and sends a single request for 
installing a driver.

>
>> +    priv->device = device;
> Wouldn't it make sense to ref it since the async operation may
> out-live the caller?

device is being ref'ed in usb-device-manager.

In V4, I used the same ref counted for the usb-connection, for the 
driver installation.
In V5, I separated them, such that the installer has its own ref/unref.
(even though it's equivalent, it seems to me people prefer it separated)


Thanks,
     Uri.
On 07/06/2012 09:46 AM, Hans de Goede wrote:
> Hi,
>
> On 07/05/2012 10:43 PM, Uri Lublin wrote:
>> - Added win-usb-driver-install.[ch]
>> - Added win-usb-clerk.h
>>
>> Operation (on Windows, spice-gtk point of view):
>> - After some sanity checks, just before redir'ing a USB device
>>    a libusb driver needs to be installed (before libusb can open the 
>> device)
>> - A connection (NamedPipe) is established with usb-clerk, a libusb
>>    driver installation service, and a request for driver installation
>>    is sent.
>> - Installation status is asynchronously read from the pipe, and
>>    spice_usb_drv_install_finished() is called.
>> - Upon a successful intallation, usbredir continues.
>>
>> Linux operation is not changed.
>> ---
>>   gtk/Makefile.am              |    3 +
>>   gtk/usb-device-manager.c     |  150 +++++++++++++++-
>>   gtk/win-usb-clerk.h          |   35 ++++
>>   gtk/win-usb-driver-install.c |  387 
>> ++++++++++++++++++++++++++++++++++++++++++
>>   gtk/win-usb-driver-install.h |   98 +++++++++++
>>   5 files changed, 663 insertions(+), 10 deletions(-)
>>   create mode 100644 gtk/win-usb-clerk.h
>>   create mode 100644 gtk/win-usb-driver-install.c
>>   create mode 100644 gtk/win-usb-driver-install.h
>>
>
> <snip>
>
>> @@ -635,7 +649,7 @@ static void 
>> spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>>               spice_usb_device_manager_connect_device_async(self,
>>                                      device, NULL,
>>                                      
>> spice_usb_device_manager_auto_connect_cb,
>> -                                   g_object_ref(device));
>> +                                   device);
>>           }
>>       }
>>
>
>
> This seems wrong as spice_usb_device_manager_auto_connect_cb will 
> still do a spice_usb_device_unref, so
> we must ref it here to match, also this seems like something which 
> belongs in a different patch...

OK.

>
>>
>> + */
>> +static void spice_usb_device_manager_drv_install_cb(GObject *gobject,
>> +                                                    GAsyncResult *res,
>> +                                                    gpointer user_data)
>> +{
>> +    SpiceUsbDeviceManager *self;
>> +    SpiceWinUsbDriver *installer;
>> +    gint status;
>> +    GError *err = NULL;
>> +    SpiceUsbDevice *device;
>> +    UsbInstallCbInfo *cbinfo;
>> +    GCancellable *cancellable;
>> +    GAsyncReadyCallback callback;
>> +
>> +    SPICE_DEBUG("Win USB driver Installation finished");
>> +
>> +    g_return_if_fail(user_data != NULL);
>> +
>> +    cbinfo = user_data;
>> +    self        = cbinfo->manager;
>> +    device      = cbinfo->device;
>> +    installer   = cbinfo->installer;
>> +    cancellable = cbinfo->cancellable;
>> +    callback    = cbinfo->callback;
>> +    user_data   = cbinfo->user_data;
>> +
>> +    g_free(cbinfo);
>> +
>> +    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
>> +    g_return_if_fail(SPICE_IS_WIN_USB_DRIVER(installer));
>> +    g_return_if_fail(device!= NULL);
>> +
>> +    status = spice_win_usb_driver_install_finish(installer, res, &err);
>> +
>> +    g_object_unref(installer);
>> +
>> +    if (err) {
>> +        g_warning("win usb driver installation failed -- %s",
>> +                  err->message);
>> +        g_error_free(err);
>> +        spice_usb_device_unref(device);
>> +        return;
>> +    }
>> +
>> +    if (!status) {
>> +        g_warning("failed to install win usb driver (status=0)");
>> +        spice_usb_device_unref(device);
>
> Can you please raise the appropriate error signal here, so that the 
> user gets
> an error dialog ?

In V5, the code does not return here on error.
Instead _spice_usb_device_manager_connect_device_async is always called 
(for install
operation). If the driver failed to install, libusb_open would fail, and 
the error would
be propagated correctly.

>
>> +        return;
>> +    }
>> +
>> +    /* device is already ref'ed */
>> +    _spice_usb_device_manager_connect_device_async(self,
>> +                                                   device,
>> +                                                   cancellable,
>> +                                                   callback,
>> +                                                   user_data);
>> +
>> +}
>> +#endif
>> +
>>   /* 
>> ------------------------------------------------------------------ */
>>   /* private 
>> api                                                        */
>>


Thanks,
     Uri.