[Spice-devel,spice-gtk,Win32,v4,03/17] Make SpiceUsbDevice a box for SpiceUsbDeviceInfo, instead of a box for libusb_device

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

Details

Message ID 1341521049-4562-4-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.
Note that this change may affect performance a bit, as sometimes there is
a need to find the libusb_device or the SpiceUsbDevice. Likely it's negligible.
---
 gtk/channel-usbredir.c        |    2 +-
 gtk/usb-device-manager-priv.h |   10 ++-
 gtk/usb-device-manager.c      |  188 +++++++++++++++++++++++++++++-----------
 3 files changed, 146 insertions(+), 54 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/channel-usbredir.c b/gtk/channel-usbredir.c
index 3d57152..354d2e1 100644
--- a/gtk/channel-usbredir.c
+++ b/gtk/channel-usbredir.c
@@ -569,7 +569,7 @@  static void do_emit_main_context(GObject *object, int event, gpointer params)
             spice_usb_device_manager_device_error(
                 spice_usb_device_manager_get(
                     spice_channel_get_session(SPICE_CHANNEL(channel)), NULL),
-                (SpiceUsbDevice *)p->device, p->error);
+                p->device, p->error);
         }
         break;
     }
diff --git a/gtk/usb-device-manager-priv.h b/gtk/usb-device-manager-priv.h
index 912e3bf..435184e 100644
--- a/gtk/usb-device-manager-priv.h
+++ b/gtk/usb-device-manager-priv.h
@@ -31,9 +31,17 @@  gboolean spice_usb_device_manager_start_event_listening(
 void spice_usb_device_manager_stop_event_listening(
     SpiceUsbDeviceManager *manager);

+#ifdef USE_USBREDIR
+#include <libusb.h>
 void spice_usb_device_manager_device_error(
-    SpiceUsbDeviceManager *manager, SpiceUsbDevice *device, GError *err);
+    SpiceUsbDeviceManager *manager, libusb_device *device, GError *err);

+guint8 spice_usb_device_get_busnum(SpiceUsbDevice *device);
+guint8 spice_usb_device_get_devaddr(SpiceUsbDevice *device);
+guint16 spice_usb_device_get_vid(SpiceUsbDevice *device);
+guint16 spice_usb_device_get_pid(SpiceUsbDevice *device);
+
+#endif
 G_END_DECLS

 #endif /* __SPICE_USB_DEVICE_MANAGER_PRIV_H__ */
diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
index c4cbc20..93bcc9b 100644
--- a/gtk/usb-device-manager.c
+++ b/gtk/usb-device-manager.c
@@ -129,13 +129,21 @@  static void spice_usb_device_unref(SpiceUsbDevice *device);

 static gboolean spice_usb_device_equal_libdev(SpiceUsbDevice *device,
                                               libusb_device *libdev);
+static SpiceUsbDevice *
+spice_usb_device_manager_libdev_to_device(SpiceUsbDeviceManager *self,
+                                          libusb_device *libdev);
+static libusb_device *
+spice_usb_device_manager_device_to_libdev(SpiceUsbDeviceManager *self,
+                                          SpiceUsbDevice *device);
+
 G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
-                    (GBoxedCopyFunc)libusb_ref_device,
-                    (GBoxedFreeFunc)libusb_unref_device)
+                    (GBoxedCopyFunc)spice_usb_device_ref,
+                    (GBoxedFreeFunc)spice_usb_device_unref)

 #else
 G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device, g_object_ref, g_object_unref)
-#endif
+#endif /* USBREDIR */
+
 static void spice_usb_device_manager_initable_iface_init(GInitableIface *iface);

 static guint signals[LAST_SIGNAL] = { 0, };
@@ -153,7 +161,7 @@  static void spice_usb_device_manager_init(SpiceUsbDeviceManager *self)
     priv->channels = g_ptr_array_new();
 #ifdef USE_USBREDIR
     priv->devices  = g_ptr_array_new_with_free_func((GDestroyNotify)
-                                                    libusb_unref_device);
+                                                    spice_usb_device_unref);
 #endif
 }

@@ -536,12 +544,12 @@  static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
                                                      gpointer      user_data)
 {
     SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(gobject);
-    libusb_device *device = user_data;
+    SpiceUsbDevice *device = user_data;
     GError *err = NULL;

     spice_usb_device_manager_connect_device_finish(self, res, &err);
     if (err) {
-        gchar *desc = spice_usb_device_get_description((SpiceUsbDevice *)device, NULL);
+        gchar *desc = spice_usb_device_get_description(device, NULL);
         g_prefix_error(&err, "Could not auto-redirect %s: ", desc);
         g_free(desc);

@@ -549,16 +557,18 @@  static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
         g_signal_emit(self, signals[AUTO_CONNECT_FAILED], 0, device, err);
         g_error_free(err);
     }
-    libusb_unref_device(device);
+    spice_usb_device_unref(device);
 }

 static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
                                              GUdevDevice            *udev)
 {
     SpiceUsbDeviceManagerPrivate *priv = self->priv;
-    libusb_device *device = NULL, **dev_list = NULL;
+    libusb_device **dev_list = NULL;
+    SpiceUsbDevice *device = NULL;
     const gchar *devtype, *devclass;
     int i, bus, address;
+    gboolean filter_ok = FALSE;

     devtype = g_udev_device_get_property(udev, "DEVTYPE");
     /* Check if this is a usb device (and not an interface) */
@@ -583,11 +593,19 @@  static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
     for (i = 0; dev_list && dev_list[i]; i++) {
         if (libusb_get_bus_number(dev_list[i]) == bus &&
             libusb_get_device_address(dev_list[i]) == address) {
-            device = libusb_ref_device(dev_list[i]);
+            device = (SpiceUsbDevice*)spice_usb_device_set_info(dev_list[i]);
             break;
         }
     }

+    if (device && priv->auto_connect) {
+        /* check filter before unref'ing dev_list[i] */
+        filter_ok = usbredirhost_check_device_filter(
+                                priv->auto_conn_filter_rules,
+                                priv->auto_conn_filter_rules_count,
+                                dev_list[i], 0) == 0;
+    }
+
     if (!priv->coldplug_list)
         libusb_free_device_list(dev_list, 1);

@@ -600,21 +618,17 @@  static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
     g_ptr_array_add(priv->devices, device);

     if (priv->auto_connect) {
-        gboolean can_redirect, auto_ok;
+        gboolean can_redirect;

         can_redirect = spice_usb_device_manager_can_redirect_device(
-                                        self, (SpiceUsbDevice *)device, NULL);
-
-        auto_ok = usbredirhost_check_device_filter(
-                            priv->auto_conn_filter_rules,
-                            priv->auto_conn_filter_rules_count,
-                            device, 0) == 0;
+                                        self, device, NULL);

-        if (can_redirect && auto_ok)
+        if (can_redirect && filter_ok) {
             spice_usb_device_manager_connect_device_async(self,
-                                   (SpiceUsbDevice *)device, NULL,
+                                   device, NULL,
                                    spice_usb_device_manager_auto_connect_cb,
-                                   libusb_ref_device(device));
+                                   g_object_ref(device));
+        }
     }

     SPICE_DEBUG("device added %p", device);
@@ -625,7 +639,7 @@  static void spice_usb_device_manager_remove_dev(SpiceUsbDeviceManager  *self,
                                                 GUdevDevice            *udev)
 {
     SpiceUsbDeviceManagerPrivate *priv = self->priv;
-    libusb_device *curr, *device = NULL;
+    SpiceUsbDevice *curr, *device = NULL;
     int bus, address;
     guint i;

@@ -634,8 +648,8 @@  static void spice_usb_device_manager_remove_dev(SpiceUsbDeviceManager  *self,

     for (i = 0; i < priv->devices->len; i++) {
         curr = g_ptr_array_index(priv->devices, i);
-        if (libusb_get_bus_number(curr) == bus &&
-               libusb_get_device_address(curr) == address) {
+        if (spice_usb_device_get_busnum(curr) == bus &&
+               spice_usb_device_get_devaddr(curr) == address) {
             device = curr;
             break;
         }
@@ -740,24 +754,32 @@  void spice_usb_device_manager_stop_event_listening(
         priv->event_thread_run = FALSE;
 }

+G_GNUC_INTERNAL
 void spice_usb_device_manager_device_error(
-    SpiceUsbDeviceManager *self, SpiceUsbDevice *device, GError *err)
+    SpiceUsbDeviceManager *self, libusb_device *libdev, GError *err)
 {
+    SpiceUsbDevice *device;
+
+    g_return_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self));
+    g_return_if_fail(libdev != 0);
+
+    device = spice_usb_device_manager_libdev_to_device(self, libdev);
+
     g_signal_emit(self, signals[DEVICE_ERROR], 0, device, err);
 }
 #endif

 static SpiceUsbredirChannel *spice_usb_device_manager_get_channel_for_dev(
-    SpiceUsbDeviceManager *manager, SpiceUsbDevice *_device)
+    SpiceUsbDeviceManager *manager, SpiceUsbDevice *device)
 {
 #ifdef USE_USBREDIR
     SpiceUsbDeviceManagerPrivate *priv = manager->priv;
-    libusb_device *device = (libusb_device *)_device;
     guint i;

     for (i = 0; i < priv->channels->len; i++) {
         SpiceUsbredirChannel *channel = g_ptr_array_index(priv->channels, i);
-        if (spice_usbredir_channel_get_device(channel) == device)
+        libusb_device *libdev = spice_usbredir_channel_get_device(channel);
+        if (spice_usb_device_equal_libdev(device, libdev))
             return channel;
     }
 #endif
@@ -817,10 +839,10 @@  GPtrArray* spice_usb_device_manager_get_devices(SpiceUsbDeviceManager *self)
     guint i;

     devices_copy = g_ptr_array_new_with_free_func((GDestroyNotify)
-                                                  libusb_unref_device);
+                                                  spice_usb_device_unref);
     for (i = 0; i < priv->devices->len; i++) {
-        libusb_device *device = g_ptr_array_index(priv->devices, i);
-        g_ptr_array_add(devices_copy, libusb_ref_device(device));
+        SpiceUsbDevice *device = g_ptr_array_index(priv->devices, i);
+        g_ptr_array_add(devices_copy, spice_usb_device_ref(device));
     }
 #endif

@@ -869,6 +891,7 @@  void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,

 #ifdef USE_USBREDIR
     SpiceUsbDeviceManagerPrivate *priv = self->priv;
+    libusb_device *libdev;
     guint i;

     if (spice_usb_device_manager_is_device_connected(self, device)) {
@@ -884,11 +907,13 @@  void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
         if (spice_usbredir_channel_get_device(channel))
             continue; /* Skip already used channels */

+        libdev = spice_usb_device_manager_device_to_libdev(self, device);
         spice_usbredir_channel_connect_device_async(channel,
-                                 (libusb_device *)device,
+                                 libdev,
                                  cancellable,
                                  spice_usb_device_manager_channel_connect_cb,
                                  result);
+        libusb_unref_device(libdev);
         return;
     }
 #endif
@@ -980,13 +1005,20 @@  spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
         g_ptr_array_index(priv->channels, 0),
         &guest_filter_rules, &guest_filter_rules_count);

-    if (guest_filter_rules &&
-            usbredirhost_check_device_filter(
+    if (guest_filter_rules) {
+        gboolean filter_ok;
+        libusb_device *ldev;
+        ldev = spice_usb_device_manager_device_to_libdev(self, device);
+        g_return_val_if_fail(ldev != NULL, FALSE);
+        filter_ok = (usbredirhost_check_device_filter(
                             guest_filter_rules, guest_filter_rules_count,
-                            (libusb_device *)device, 0) != 0) {
-        g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
-                            _("Some USB devices are blocked by host policy"));
-        return FALSE;
+                            ldev, 0) == 0);
+        libusb_unref_device(ldev);
+        if (!filter_ok) {
+            g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
+                                _("Some USB devices are blocked by host policy"));
+            return FALSE;
+        }
     }

     /* Check if there are free channels */
@@ -1030,34 +1062,32 @@  spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
  *
  * Returns: a newly-allocated string holding the description, or %NULL if failed
  */
-gchar *spice_usb_device_get_description(SpiceUsbDevice *_device, const gchar *format)
+gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar *format)
 {
 #ifdef USE_USBREDIR
-    libusb_device *device = (libusb_device *)_device;
-    struct libusb_device_descriptor desc;
-    int bus, address;
+    int bus, addr, vid, pid;
     gchar *description, *descriptor, *manufacturer = NULL, *product = NULL;

-    g_return_val_if_fail(device != NULL, NULL);
+    g_return_val_if_fail(device!= NULL, NULL);

-    bus     = libusb_get_bus_number(device);
-    address = libusb_get_device_address(device);
+    bus  = spice_usb_device_get_busnum(device);
+    addr = spice_usb_device_get_devaddr(device);
+    vid  = spice_usb_device_get_vid(device);
+    pid  = spice_usb_device_get_pid(device);

-    if (libusb_get_device_descriptor(device, &desc) == LIBUSB_SUCCESS) {
-        spice_usb_util_get_device_strings(bus, address,
-                                          desc.idVendor, desc.idProduct,
-                                          &manufacturer, &product);
-        descriptor = g_strdup_printf("[%04x:%04x]", desc.idVendor, desc.idProduct);
+    if ((vid > 0) && (pid > 0)) {
+        descriptor = g_strdup_printf("[%04x:%04x]", vid, pid);
     } else {
-        spice_usb_util_get_device_strings(bus, address, -1, -1,
-                                          &manufacturer, &product);
         descriptor = g_strdup("");
     }

+    spice_usb_util_get_device_strings(bus, addr, vid, pid,
+                                      &manufacturer, &product);
+
     if (!format)
         format = _("%s %s %s at %d-%d");

-    description = g_strdup_printf(format, manufacturer, product, descriptor, bus, address);
+    description = g_strdup_printf(format, manufacturer, product, descriptor, bus, addr);

     g_free(manufacturer);
     g_free(descriptor);
@@ -1070,7 +1100,6 @@  gchar *spice_usb_device_get_description(SpiceUsbDevice *_device, const gchar *fo
 }


-
 #ifdef USE_USBREDIR
 /*
  * SpiceUsbDeviceInfo
@@ -1196,4 +1225,59 @@  spice_usb_device_equal_libdev(SpiceUsbDevice *device,

     return ((bus1 == bus2) && (addr1 == addr2));
 }
+
+static SpiceUsbDevice *
+spice_usb_device_manager_libdev_to_device(SpiceUsbDeviceManager *self,
+                                          libusb_device *libdev)
+{
+    SpiceUsbDeviceManagerPrivate *priv = self->priv;
+    SpiceUsbDevice *device = NULL;
+    int i;
+
+    for (i = 0; i < priv->devices->len; i++) {
+        device = g_ptr_array_index(priv->devices, i);
+        if (spice_usb_device_equal_libdev(device, libdev)) {
+            break; /* found it */
+        }
+        device = NULL; /* did not find it yet */
+    }
+    return device;
+}
+
+/*
+ * Caller must libusb_unref_device the libusb_device returned by this function.
+ * Returns a libusb_device, or NULL upon failure
+ */
+static libusb_device *
+spice_usb_device_manager_device_to_libdev(SpiceUsbDeviceManager *self,
+                                          SpiceUsbDevice *device)
+{
+    libusb_device *d, **devlist;
+    guint8 bus, addr;
+    int i;
+
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self), NULL);
+    g_return_val_if_fail(device != NULL, NULL);
+    g_return_val_if_fail(self->priv != NULL, NULL);
+    g_return_val_if_fail(self->priv->context != NULL, NULL);
+
+    bus  = spice_usb_device_get_busnum(device);
+    addr = spice_usb_device_get_devaddr(device);
+
+    libusb_get_device_list(self->priv->context, &devlist);
+    if (!devlist)
+        return NULL;
+
+    for (i = 0; (d = devlist[i]) != NULL; i++) {
+        if ((libusb_get_bus_number(d) == bus) &&
+            (libusb_get_device_address(d) == addr)) {
+            libusb_ref_device(d);
+            break;
+        }
+    }
+
+    libusb_free_device_list(devlist, 1);
+
+    return d;
+}
 #endif /* USE_USBREDIR */

Comments

On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin <uril@redhat.com> wrote:
> +    gboolean filter_ok = FALSE;

Actually, I think "auto_ok" was a more readable name here, since it
affects only the "auto" code path.
On Thu, Jul 5, 2012 at 11:26 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin <uril@redhat.com> wrote:
>> +    gboolean filter_ok = FALSE;
>
> Actually, I think "auto_ok" was a more readable name here, since it
> affects only the "auto" code path.
>
> --
> Marc-André Lureau

+        if (can_redirect && filter_ok) {
             spice_usb_device_manager_connect_device_async(self,
-                                   (SpiceUsbDevice *)device, NULL,
+                                   device, NULL,
                                    spice_usb_device_manager_auto_connect_cb,
-                                   libusb_ref_device(device));
+                                   g_object_ref(device));
+        }

Is  the  g_object_ref() is a left over from a previous version ?
Hi,

On 07/05/2012 10:43 PM, Uri Lublin wrote:
> Note that this change may affect performance a bit, as sometimes there is
> a need to find the libusb_device or the SpiceUsbDevice. Likely it's negligible.
> ---
>   gtk/channel-usbredir.c        |    2 +-
>   gtk/usb-device-manager-priv.h |   10 ++-
>   gtk/usb-device-manager.c      |  188 +++++++++++++++++++++++++++++-----------
>   3 files changed, 146 insertions(+), 54 deletions(-)
>
> diff --git a/gtk/channel-usbredir.c b/gtk/channel-usbredir.c
> index 3d57152..354d2e1 100644
> --- a/gtk/channel-usbredir.c
> +++ b/gtk/channel-usbredir.c

<snip>

> @@ -549,16 +557,18 @@ static void spice_usb_device_manager_auto_connect_cb(GObject      *gobject,
>           g_signal_emit(self, signals[AUTO_CONNECT_FAILED], 0, device, err);
>           g_error_free(err);
>       }
> -    libusb_unref_device(device);
> +    spice_usb_device_unref(device);
>   }
>
>   static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>                                                GUdevDevice            *udev)
>   {
>       SpiceUsbDeviceManagerPrivate *priv = self->priv;
> -    libusb_device *device = NULL, **dev_list = NULL;
> +    libusb_device **dev_list = NULL;
> +    SpiceUsbDevice *device = NULL;
>       const gchar *devtype, *devclass;
>       int i, bus, address;
> +    gboolean filter_ok = FALSE;
>
>       devtype = g_udev_device_get_property(udev, "DEVTYPE");
>       /* Check if this is a usb device (and not an interface) */
> @@ -583,11 +593,19 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>       for (i = 0; dev_list && dev_list[i]; i++) {
>           if (libusb_get_bus_number(dev_list[i]) == bus &&
>               libusb_get_device_address(dev_list[i]) == address) {
> -            device = libusb_ref_device(dev_list[i]);
> +            device = (SpiceUsbDevice*)spice_usb_device_set_info(dev_list[i]);
>               break;
>           }
>       }
>
> +    if (device && priv->auto_connect) {
> +        /* check filter before unref'ing dev_list[i] */
> +        filter_ok = usbredirhost_check_device_filter(
> +                                priv->auto_conn_filter_rules,
> +                                priv->auto_conn_filter_rules_count,
> +                                dev_list[i], 0) == 0;
> +    }
> +
>       if (!priv->coldplug_list)
>           libusb_free_device_list(dev_list, 1);
>
> @@ -600,21 +618,17 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>       g_ptr_array_add(priv->devices, device);
>
>       if (priv->auto_connect) {
> -        gboolean can_redirect, auto_ok;
> +        gboolean can_redirect;
>
>           can_redirect = spice_usb_device_manager_can_redirect_device(
> -                                        self, (SpiceUsbDevice *)device, NULL);
> -
> -        auto_ok = usbredirhost_check_device_filter(
> -                            priv->auto_conn_filter_rules,
> -                            priv->auto_conn_filter_rules_count,
> -                            device, 0) == 0;
> +                                        self, device, NULL);
>
> -        if (can_redirect && auto_ok)
> +        if (can_redirect && filter_ok) {
>               spice_usb_device_manager_connect_device_async(self,
> -                                   (SpiceUsbDevice *)device, NULL,
> +                                   device, NULL,
>                                      spice_usb_device_manager_auto_connect_cb,
> -                                   libusb_ref_device(device));
> +                                   g_object_ref(device));
> +        }
>       }
>
>       SPICE_DEBUG("device added %p", device);

The g_object_ref here must be a spice_usb_device_ref!!

<snip>

> @@ -980,13 +1005,20 @@ spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
>           g_ptr_array_index(priv->channels, 0),
>           &guest_filter_rules, &guest_filter_rules_count);
>
> -    if (guest_filter_rules &&
> -            usbredirhost_check_device_filter(
> +    if (guest_filter_rules) {
> +        gboolean filter_ok;
> +        libusb_device *ldev;
> +        ldev = spice_usb_device_manager_device_to_libdev(self, device);
> +        g_return_val_if_fail(ldev != NULL, FALSE);
> +        filter_ok = (usbredirhost_check_device_filter(
>                               guest_filter_rules, guest_filter_rules_count,
> -                            (libusb_device *)device, 0) != 0) {
> -        g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> -                            _("Some USB devices are blocked by host policy"));
> -        return FALSE;
> +                            ldev, 0) == 0);
> +        libusb_unref_device(ldev);
> +        if (!filter_ok) {
> +            g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> +                                _("Some USB devices are blocked by host policy"));
> +            return FALSE;
> +        }
>       }
>
>       /* Check if there are free channels */

You're using libdev for the actual libusb_device everywhere and now here you are using ldev, please
make it libdev for consistency.

Regards,

Hans
On 07/06/2012 12:26 AM, Marc-André Lureau wrote:
> On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin<uril@redhat.com>  wrote:
>> +    gboolean filter_ok = FALSE;
> Actually, I think "auto_ok" was a more readable name here, since it
> affects only the "auto" code path.
>

I renamed it "filter_ok" as it checks the filter.

Now it's back to "auto_ok".

Thanks,
     Uri.
On 07/06/2012 12:46 AM, Marc-André Lureau wrote:
> On Thu, Jul 5, 2012 at 11:26 PM, Marc-André Lureau
> <marcandre.lureau@gmail.com>  wrote:
>> On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin<uril@redhat.com>  wrote:
>>> +    gboolean filter_ok = FALSE;
>> Actually, I think "auto_ok" was a more readable name here, since it
>> affects only the "auto" code path.
>>
>> --
>> Marc-André Lureau
> +        if (can_redirect&&  filter_ok) {
>               spice_usb_device_manager_connect_device_async(self,
> -                                   (SpiceUsbDevice *)device, NULL,
> +                                   device, NULL,
>                                      spice_usb_device_manager_auto_connect_cb,
> -                                   libusb_ref_device(device));
> +                                   g_object_ref(device));
> +        }
>
> Is  the  g_object_ref() is a left over from a previous version ?
>
>

Yes, fixed now.

Thanks,
     Uri.
On 07/06/2012 09:40 AM, Hans de Goede wrote:
>
>> -    if (guest_filter_rules &&
>> -            usbredirhost_check_device_filter(
>> +    if (guest_filter_rules) {
>> +        gboolean filter_ok;
>> +        libusb_device *ldev;
>> +        ldev = spice_usb_device_manager_device_to_libdev(self, device);
>> +        g_return_val_if_fail(ldev != NULL, FALSE);
>> +        filter_ok = (usbredirhost_check_device_filter(
>>                               guest_filter_rules, 
>> guest_filter_rules_count,
>> -                            (libusb_device *)device, 0) != 0) {
>> -        g_set_error_literal(err, SPICE_CLIENT_ERROR, 
>> SPICE_CLIENT_ERROR_FAILED,
>> -                            _("Some USB devices are blocked by host 
>> policy"));
>> -        return FALSE;
>> +                            ldev, 0) == 0);
>> +        libusb_unref_device(ldev);
>> +        if (!filter_ok) {
>> +            g_set_error_literal(err, SPICE_CLIENT_ERROR, 
>> SPICE_CLIENT_ERROR_FAILED,
>> +                                _("Some USB devices are blocked by 
>> host policy"));
>> +            return FALSE;
>> +        }
>>       }
>>
>>       /* Check if there are free channels */
>
> You're using libdev for the actual libusb_device everywhere and now 
> here you are using ldev, please
> make it libdev for consistency.
>

Fixed both,

Thanks,
     Uri.