[Spice-devel,spice-gtk,Win32,v3,05/12] Make SpiceUsbDevice a gobject, instead of a box for libusb_device

Submitted by Uri Lublin on June 28, 2012, 1:46 a.m.

Details

Message ID 1340848001-7791-6-git-send-email-uril@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin June 28, 2012, 1:46 a.m.
Note that this change may affect performance a bit, as we now need to look
for the libusb_device if we need it. Likely it's negligible.
---
 gtk/Makefile.am               |    4 +
 gtk/channel-usbredir-priv.h   |    4 +-
 gtk/channel-usbredir.c        |   46 +++++-----
 gtk/map-file                  |    7 ++
 gtk/usb-device-manager-priv.h |    3 +
 gtk/usb-device-manager.c      |  194 +++++++++++++++++++++++++++++------------
 gtk/usb-device-manager.h      |    5 +-
 gtk/usb-device-widget.c       |    9 +--
 spice-common                  |    2 +-
 9 files changed, 182 insertions(+), 92 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/Makefile.am b/gtk/Makefile.am
index 0327d65..e79abae 100644
--- a/gtk/Makefile.am
+++ b/gtk/Makefile.am
@@ -228,6 +228,9 @@  libspice_client_glib_2_0_la_SOURCES =			\
 	channel-usbredir-priv.h				\
 	smartcard-manager.c				\
 	smartcard-manager-priv.h			\
+	spice-usb-device.c				\
+	spice-usb-device.h				\
+	spice-usb-device-priv.h				\
 	usb-device-manager.c				\
 	usb-device-manager-priv.h			\
 	usbutil.c					\
@@ -266,6 +269,7 @@  libspice_client_glibinclude_HEADERS =	\
 	channel-record.h		\
 	channel-smartcard.h		\
 	channel-usbredir.h		\
+	spice-usb-device.h		\
 	usb-device-manager.h		\
 	smartcard-manager.h		\
 	$(NULL)
diff --git a/gtk/channel-usbredir-priv.h b/gtk/channel-usbredir-priv.h
index 5d28c79..e6cd538 100644
--- a/gtk/channel-usbredir-priv.h
+++ b/gtk/channel-usbredir-priv.h
@@ -37,7 +37,7 @@  void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
    (through spice_channel_connect()), before calling this. */
 void spice_usbredir_channel_connect_device_async(
                                         SpiceUsbredirChannel *channel,
-                                        libusb_device        *device,
+                                        SpiceUsbDevice       *device,
                                         GCancellable         *cancellable,
                                         GAsyncReadyCallback   callback,
                                         gpointer              user_data);
@@ -48,7 +48,7 @@  gboolean spice_usbredir_channel_connect_device_finish(

 void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);

-libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);
+SpiceUsbDevice *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);

 void spice_usbredir_channel_get_guest_filter(
                           SpiceUsbredirChannel               *channel,
diff --git a/gtk/channel-usbredir.c b/gtk/channel-usbredir.c
index 3d57152..63efb77 100644
--- a/gtk/channel-usbredir.c
+++ b/gtk/channel-usbredir.c
@@ -35,6 +35,7 @@ 
 #include "spice-client.h"
 #include "spice-common.h"

+#include "spice-usb-device-priv.h"
 #include "spice-channel-priv.h"
 #include "glib-compat.h"

@@ -65,7 +66,7 @@  enum SpiceUsbredirChannelState {
 };

 struct _SpiceUsbredirChannelPrivate {
-    libusb_device *device;
+    SpiceUsbDevice *device;
     struct usbredirhost *host;
     /* To catch usbredirhost error messages and report them as a GError */
     GError **catch_error;
@@ -211,6 +212,7 @@  static gboolean spice_usbredir_channel_open_device(
     SpiceUsbredirChannel *channel, GError **err)
 {
     SpiceUsbredirChannelPrivate *priv = channel->priv;
+    SpiceUsbDeviceManager *manager;
     libusb_device_handle *handle = NULL;
     int rc, status;

@@ -220,7 +222,11 @@  static gboolean spice_usbredir_channel_open_device(
 #endif
                          , FALSE);

-    rc = libusb_open(priv->device, &handle);
+    manager = spice_usb_device_manager_get(
+                    spice_channel_get_session(SPICE_CHANNEL(channel)), err);
+    if (!manager)
+        return FALSE;
+    rc = spice_usb_device_libusb_open(manager, priv->device, &handle);
     if (rc != 0) {
         g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
                     "Could not open usb device: %s [%i]",
@@ -236,10 +242,7 @@  static gboolean spice_usbredir_channel_open_device(
         return FALSE;
     }

-    if (!spice_usb_device_manager_start_event_listening(
-            spice_usb_device_manager_get(
-                spice_channel_get_session(SPICE_CHANNEL(channel)), NULL),
-            err)) {
+    if (!spice_usb_device_manager_start_event_listening(manager, err)) {
         usbredirhost_set_device(priv->host, NULL);
         return FALSE;
     }
@@ -272,7 +275,7 @@  static void spice_usbredir_channel_open_acl_cb(
     }
     if (err) {
         g_simple_async_result_take_error(priv->result, err);
-        libusb_unref_device(priv->device);
+        g_object_unref(priv->device);
         priv->device = NULL;
         priv->state  = STATE_DISCONNECTED;
     }
@@ -290,7 +293,7 @@  static void spice_usbredir_channel_open_acl_cb(
 G_GNUC_INTERNAL
 void spice_usbredir_channel_connect_device_async(
                                           SpiceUsbredirChannel *channel,
-                                          libusb_device        *device,
+                                          SpiceUsbDevice       *device,
                                           GCancellable         *cancellable,
                                           GAsyncReadyCallback   callback,
                                           gpointer              user_data)
@@ -302,7 +305,7 @@  void spice_usbredir_channel_connect_device_async(
 #endif

     g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
-    g_return_if_fail(device != NULL);
+    g_return_if_fail(SPICE_IS_USB_DEVICE(device));

     SPICE_DEBUG("connecting usb channel %p", channel);

@@ -323,7 +326,7 @@  void spice_usbredir_channel_connect_device_async(
         goto done;
     }

-    priv->device = libusb_ref_device(device);
+    priv->device = g_object_ref(device);
 #if USE_POLKIT
     priv->result = result;
     priv->state  = STATE_WAITING_FOR_ACL_HELPER;
@@ -331,8 +334,8 @@  void spice_usbredir_channel_connect_device_async(
     g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
                  "inhibit-keyboard-grab", TRUE, NULL);
     spice_usb_acl_helper_open_acl(priv->acl_helper,
-                                  libusb_get_bus_number(device),
-                                  libusb_get_device_address(device),
+                                  spice_usb_device_get_busnum(device),
+                                  spice_usb_device_get_devaddr(device),
                                   cancellable,
                                   spice_usbredir_channel_open_acl_cb,
                                   channel);
@@ -340,7 +343,7 @@  void spice_usbredir_channel_connect_device_async(
 #else
     if (!spice_usbredir_channel_open_device(channel, &err)) {
         g_simple_async_result_take_error(result, err);
-        libusb_unref_device(priv->device);
+        g_object_unref(priv->device);
         priv->device = NULL;
     }
 #endif
@@ -398,7 +401,7 @@  void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
                 spice_channel_get_session(SPICE_CHANNEL(channel)), NULL));
         /* This also closes the libusb handle we passed from open_device */
         usbredirhost_set_device(priv->host, NULL);
-        libusb_unref_device(priv->device);
+        g_object_unref(priv->device);
         priv->device = NULL;
         priv->state  = STATE_DISCONNECTED;
         break;
@@ -406,7 +409,7 @@  void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
 }

 G_GNUC_INTERNAL
-libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel)
+SpiceUsbDevice *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel)
 {
     return channel->priv->device;
 }
@@ -550,7 +553,7 @@  enum {
 };

 struct DEVICE_ERROR {
-    libusb_device *device;
+    SpiceUsbDevice *device;
     GError *error;
 };

@@ -569,7 +572,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;
     }
@@ -624,14 +627,13 @@  static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)

     r = usbredirhost_read_guest_data(priv->host);
     if (r != 0) {
-        libusb_device *device = priv->device;
+        SpiceUsbDevice *device = priv->device;
         gchar *desc;
         GError *err;

         g_return_if_fail(device != NULL);

-        desc = spice_usb_device_get_description((SpiceUsbDevice *)device,
-                                                NULL);
+        desc = spice_usb_device_get_description(device, NULL);
         switch (r) {
         case usbredirhost_read_parse_error:
             err = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
@@ -655,9 +657,9 @@  static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)

         SPICE_DEBUG("%s", err->message);

-        g_boxed_copy(spice_usb_device_get_type(), device);
+        g_object_ref(device);
         emit_main_context(channel, DEVICE_ERROR, device, err);
-        g_boxed_free(spice_usb_device_get_type(), device);
+        g_object_unref(device);

         g_error_free(err);
     }
diff --git a/gtk/map-file b/gtk/map-file
index 32ead37..7f18d19 100644
--- a/gtk/map-file
+++ b/gtk/map-file
@@ -100,6 +100,13 @@  spice_usbredir_channel_get_type;
 spice_util_get_debug;
 spice_util_get_version_string;
 spice_util_set_debug;
+spice_usb_device_get_type;
+spice_usb_device_new;
+spice_usb_device_set_info;
+spice_usb_device_get_busnum;
+spice_usb_device_get_devaddr;
+spice_usb_device_get_vid;
+spice_usb_device_get_pid;
 local:
 *;
 };
diff --git a/gtk/usb-device-manager-priv.h b/gtk/usb-device-manager-priv.h
index 912e3bf..6da890c 100644
--- a/gtk/usb-device-manager-priv.h
+++ b/gtk/usb-device-manager-priv.h
@@ -34,6 +34,9 @@  void spice_usb_device_manager_stop_event_listening(
 void spice_usb_device_manager_device_error(
     SpiceUsbDeviceManager *manager, SpiceUsbDevice *device, GError *err);

+int spice_usb_device_libusb_open(SpiceUsbDeviceManager *self,
+                                 SpiceUsbDevice *device,
+                                 struct libusb_device_handle **handle);
 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 25cb14a..4448a74 100644
--- a/gtk/usb-device-manager.c
+++ b/gtk/usb-device-manager.c
@@ -38,6 +38,8 @@ 
 #include "spice-client.h"
 #include "spice-marshal.h"
 #include "usb-device-manager-priv.h"
+#include "spice-usb-device.h"
+#include "spice-usb-device-priv.h"

 #include <glib/gi18n.h>

@@ -112,14 +114,11 @@  static void spice_usb_device_manager_uevent_cb(GUdevClient     *client,
                                                gpointer         user_data);
 static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
                                              GUdevDevice            *udev);
+static
+libusb_device *spice_usb_device_find_libusb_device(SpiceUsbDeviceManager *self,
+                                                   SpiceUsbDevice *device);
+#endif /* ifdef USBREDIR */

-G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
-                    (GBoxedCopyFunc)libusb_ref_device,
-                    (GBoxedFreeFunc)libusb_unref_device)
-
-#else
-G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device, g_object_ref, g_object_unref)
-#endif
 static void spice_usb_device_manager_initable_iface_init(GInitableIface *iface);

 static guint signals[LAST_SIGNAL] = { 0, };
@@ -127,6 +126,8 @@  static guint signals[LAST_SIGNAL] = { 0, };
 G_DEFINE_TYPE_WITH_CODE(SpiceUsbDeviceManager, spice_usb_device_manager, G_TYPE_OBJECT,
      G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, spice_usb_device_manager_initable_iface_init));

+
+
 static void spice_usb_device_manager_init(SpiceUsbDeviceManager *self)
 {
     SpiceUsbDeviceManagerPrivate *priv;
@@ -137,7 +138,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);
+                                                    g_object_unref);
 #endif
 }

@@ -395,7 +396,7 @@  static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
                      g_cclosure_marshal_VOID__BOXED,
                      G_TYPE_NONE,
                      1,
-                     SPICE_TYPE_USB_DEVICE);
+                     G_TYPE_POINTER);

     /**
      * SpiceUsbDeviceManager::device-removed:
@@ -414,7 +415,7 @@  static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
                      g_cclosure_marshal_VOID__BOXED,
                      G_TYPE_NONE,
                      1,
-                     SPICE_TYPE_USB_DEVICE);
+                     G_TYPE_POINTER);

     /**
      * SpiceUsbDeviceManager::auto-connect-failed:
@@ -435,7 +436,7 @@  static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
                      g_cclosure_user_marshal_VOID__BOXED_BOXED,
                      G_TYPE_NONE,
                      2,
-                     SPICE_TYPE_USB_DEVICE,
+                     G_TYPE_POINTER,
                      G_TYPE_ERROR);

     /**
@@ -457,7 +458,7 @@  static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
                      g_cclosure_user_marshal_VOID__BOXED_BOXED,
                      G_TYPE_NONE,
                      2,
-                     SPICE_TYPE_USB_DEVICE,
+                     G_TYPE_POINTER,
                      G_TYPE_ERROR);

     g_type_class_add_private(klass, sizeof(SpiceUsbDeviceManagerPrivate));
@@ -515,12 +516,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);

@@ -528,16 +529,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);
+    g_object_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) */
@@ -562,7 +565,12 @@  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 = spice_usb_device_new();
+            spice_usb_device_set_info(device, 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;
             break;
         }
     }
@@ -579,21 +587,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);
@@ -604,7 +608,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;

@@ -613,8 +617,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;
         }
@@ -727,11 +731,10 @@  void spice_usb_device_manager_device_error(
 #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++) {
@@ -796,10 +799,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);
+                                                  g_object_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, g_object_ref(device));
     }
 #endif

@@ -864,7 +867,7 @@  void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
             continue; /* Skip already used channels */

         spice_usbredir_channel_connect_device_async(channel,
-                                 (libusb_device *)device,
+                                 device,
                                  cancellable,
                                  spice_usb_device_manager_channel_connect_cb,
                                  result);
@@ -959,13 +962,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_find_libusb_device(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 */
@@ -1009,34 +1019,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(SPICE_IS_USB_DEVICE(device), 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);
@@ -1047,3 +1055,75 @@  gchar *spice_usb_device_get_description(SpiceUsbDevice *_device, const gchar *fo
     return NULL;
 #endif
 }
+
+
+
+/*
+ * 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_find_libusb_device(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(SPICE_IS_USB_DEVICE(device), 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;
+}
+
+int spice_usb_device_libusb_open(SpiceUsbDeviceManager *self,
+                                 SpiceUsbDevice *device,
+                                 struct libusb_device_handle **handle)
+{
+    libusb_device *ldev;
+    guint8 bus, addr;
+    int rc;
+
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self), -1);
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), -2);
+
+    bus  = spice_usb_device_get_busnum(device);
+    addr = spice_usb_device_get_devaddr(device);
+
+    ldev = spice_usb_device_find_libusb_device(self, device);
+    if (ldev) {
+        rc = libusb_open(ldev, handle);
+        if (rc) {
+            const gchar *errstr = spice_usbutil_libusb_strerror(rc);
+            g_warning("Failed to open usb device %d.%d -- %s (%i)",
+                      bus, addr, errstr, rc);
+        }
+        libusb_unref_device(ldev);
+    } else {
+        rc = -3;
+        g_warning("Did not find device %d.%d to open", bus, addr);
+    }
+
+
+
+    return rc;
+}
diff --git a/gtk/usb-device-manager.h b/gtk/usb-device-manager.h
index df138ee..e5577d8 100644
--- a/gtk/usb-device-manager.h
+++ b/gtk/usb-device-manager.h
@@ -23,6 +23,7 @@ 

 #include "spice-client.h"
 #include <gio/gio.h>
+#include "spice-usb-device.h"

 G_BEGIN_DECLS

@@ -33,13 +34,12 @@  G_BEGIN_DECLS
 #define SPICE_IS_USB_DEVICE_MANAGER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), SPICE_TYPE_USB_DEVICE_MANAGER))
 #define SPICE_USB_DEVICE_MANAGER_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), SPICE_TYPE_USB_DEVICE_MANAGER, SpiceUsbDeviceManagerClass))

-#define SPICE_TYPE_USB_DEVICE                    (spice_usb_device_get_type())
+

 typedef struct _SpiceUsbDeviceManager SpiceUsbDeviceManager;
 typedef struct _SpiceUsbDeviceManagerClass SpiceUsbDeviceManagerClass;
 typedef struct _SpiceUsbDeviceManagerPrivate SpiceUsbDeviceManagerPrivate;

-typedef struct _SpiceUsbDevice SpiceUsbDevice;

 /**
  * SpiceUsbDeviceManager:
@@ -85,7 +85,6 @@  struct _SpiceUsbDeviceManagerClass
     gchar _spice_reserved[SPICE_RESERVED_PADDING];
 };

-GType spice_usb_device_get_type(void);
 GType spice_usb_device_manager_get_type(void);

 gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar *format);
diff --git a/gtk/usb-device-widget.c b/gtk/usb-device-widget.c
index 3ed81e4..c342981 100644
--- a/gtk/usb-device-widget.c
+++ b/gtk/usb-device-widget.c
@@ -465,11 +465,6 @@  static void checkbox_clicked_cb(GtkWidget *check, gpointer user_data)
     spice_usb_device_widget_update_status(self);
 }

-static void checkbox_usb_device_destroy_notify(gpointer data)
-{
-    g_boxed_free(spice_usb_device_get_type(), data);
-}
-
 static void device_added_cb(SpiceUsbDeviceManager *manager,
     SpiceUsbDevice *device, gpointer user_data)
 {
@@ -489,8 +484,8 @@  static void device_added_cb(SpiceUsbDeviceManager *manager,

     g_object_set_data_full(
             G_OBJECT(check), "usb-device",
-            g_boxed_copy(spice_usb_device_get_type(), device),
-            checkbox_usb_device_destroy_notify);
+            g_object_ref(device),
+            g_object_unref);
     g_signal_connect(G_OBJECT(check), "clicked",
                      G_CALLBACK(checkbox_clicked_cb), self);

diff --git a/spice-common b/spice-common
index 5f44094..22fc0b0 160000
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@ 
-Subproject commit 5f4409494066b5f59df58d6207fdbb0441aa9e90
+Subproject commit 22fc0b0145876b90385c1c88923bcd72a6380812

Comments

On Thu, Jun 28, 2012 at 04:46:34AM +0300, Uri Lublin wrote:
> Note that this change may affect performance a bit, as we now need to look
> for the libusb_device if we need it. Likely it's negligible.
> ---
>  gtk/Makefile.am               |    4 +
>  gtk/channel-usbredir-priv.h   |    4 +-
>  gtk/channel-usbredir.c        |   46 +++++-----
>  gtk/map-file                  |    7 ++
>  gtk/usb-device-manager-priv.h |    3 +
>  gtk/usb-device-manager.c      |  194 +++++++++++++++++++++++++++++------------
>  gtk/usb-device-manager.h      |    5 +-
>  gtk/usb-device-widget.c       |    9 +--
>  spice-common                  |    2 +-
>  9 files changed, 182 insertions(+), 92 deletions(-)
> 
> @@ -395,7 +396,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_marshal_VOID__BOXED,
>                       G_TYPE_NONE,
>                       1,
> -                     SPICE_TYPE_USB_DEVICE);
> +                     G_TYPE_POINTER);

Why are you not keeping SPICE_TYPE_USB_DEVICE here and in the calls below?

> 
>      /**
>       * SpiceUsbDeviceManager::device-removed:
> @@ -414,7 +415,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_marshal_VOID__BOXED,
>                       G_TYPE_NONE,
>                       1,
> -                     SPICE_TYPE_USB_DEVICE);
> +                     G_TYPE_POINTER);
> 
>      /**
>       * SpiceUsbDeviceManager::auto-connect-failed:
> @@ -435,7 +436,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_user_marshal_VOID__BOXED_BOXED,
>                       G_TYPE_NONE,
>                       2,
> -                     SPICE_TYPE_USB_DEVICE,
> +                     G_TYPE_POINTER,
>                       G_TYPE_ERROR);
> 
>      /**
> @@ -457,7 +458,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_user_marshal_VOID__BOXED_BOXED,
>                       G_TYPE_NONE,
>                       2,
> -                     SPICE_TYPE_USB_DEVICE,
> +                     G_TYPE_POINTER,
>                       G_TYPE_ERROR);
> 
>      g_type_class_add_private(klass, sizeof(SpiceUsbDeviceManagerPrivate));

> @@ -1009,34 +1019,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(SPICE_IS_USB_DEVICE(device), 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);

I don't think this will do the right thing in error cases, get_vid returns
an uint16_t which value is 0xffff when there's an error, it will be turned
into 65535 when it's cast to int, it won't be -1. I haven't looked at the
rest of the patch for similar issues.

Christophe
On Thu, Jun 28, 2012 at 3:46 AM, Uri Lublin <uril@redhat.com> wrote:
> -#define SPICE_TYPE_USB_DEVICE                    (spice_usb_device_get_type())
> +
>
>  typedef struct _SpiceUsbDeviceManager SpiceUsbDeviceManager;
>  typedef struct _SpiceUsbDeviceManagerClass SpiceUsbDeviceManagerClass;
>  typedef struct _SpiceUsbDeviceManagerPrivate SpiceUsbDeviceManagerPrivate;
>
> -typedef struct _SpiceUsbDevice SpiceUsbDevice;
>
>  /**
>  * SpiceUsbDeviceManager:
> @@ -85,7 +85,6 @@ struct _SpiceUsbDeviceManagerClass
>     gchar _spice_reserved[SPICE_RESERVED_PADDING];
>  };
>
> -GType spice_usb_device_get_type(void);

This change will perhaps not break API, but you are definetly going to
break ABI if you replace boxed pointers with objects in public API.
Seems great to me, but few comments below.

Uri Lublin wrote:
> Note that this change may affect performance a bit, as we now need to look
> for the libusb_device if we need it. Likely it's negligible.
> ---
>  gtk/Makefile.am               |    4 +
>  gtk/channel-usbredir-priv.h   |    4 +-
>  gtk/channel-usbredir.c        |   46 +++++-----
>  gtk/map-file                  |    7 ++
>  gtk/usb-device-manager-priv.h |    3 +
>  gtk/usb-device-manager.c      |  194 +++++++++++++++++++++++++++++------------
>  gtk/usb-device-manager.h      |    5 +-
>  gtk/usb-device-widget.c       |    9 +--
>  spice-common                  |    2 +-
>  9 files changed, 182 insertions(+), 92 deletions(-)
>
> diff --git a/gtk/Makefile.am b/gtk/Makefile.am
> index 0327d65..e79abae 100644
> --- a/gtk/Makefile.am
> +++ b/gtk/Makefile.am
> @@ -228,6 +228,9 @@ libspice_client_glib_2_0_la_SOURCES =			\
>  	channel-usbredir-priv.h				\
>  	smartcard-manager.c				\
>  	smartcard-manager-priv.h			\
> +	spice-usb-device.c				\
> +	spice-usb-device.h				\
> +	spice-usb-device-priv.h				\
>  	usb-device-manager.c				\
>  	usb-device-manager-priv.h			\
>  	usbutil.c					\
> @@ -266,6 +269,7 @@ libspice_client_glibinclude_HEADERS =	\
>  	channel-record.h		\
>  	channel-smartcard.h		\
>  	channel-usbredir.h		\
> +	spice-usb-device.h		\
>  	usb-device-manager.h		\
>  	smartcard-manager.h		\
>  	$(NULL)
> diff --git a/gtk/channel-usbredir-priv.h b/gtk/channel-usbredir-priv.h
> index 5d28c79..e6cd538 100644
> --- a/gtk/channel-usbredir-priv.h
> +++ b/gtk/channel-usbredir-priv.h
> @@ -37,7 +37,7 @@ void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
>     (through spice_channel_connect()), before calling this. */
>  void spice_usbredir_channel_connect_device_async(
>                                          SpiceUsbredirChannel *channel,
> -                                        libusb_device        *device,
> +                                        SpiceUsbDevice       *device,
>                                          GCancellable         *cancellable,
>                                          GAsyncReadyCallback   callback,
>                                          gpointer              user_data);
> @@ -48,7 +48,7 @@ gboolean spice_usbredir_channel_connect_device_finish(
>
>  void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel);
>
> -libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);
> +SpiceUsbDevice *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel);
>
>  void spice_usbredir_channel_get_guest_filter(
>                            SpiceUsbredirChannel               *channel,
> diff --git a/gtk/channel-usbredir.c b/gtk/channel-usbredir.c
> index 3d57152..63efb77 100644
> --- a/gtk/channel-usbredir.c
> +++ b/gtk/channel-usbredir.c
> @@ -35,6 +35,7 @@
>  #include "spice-client.h"
>  #include "spice-common.h"
>
> +#include "spice-usb-device-priv.h"
>  #include "spice-channel-priv.h"
>  #include "glib-compat.h"
>
> @@ -65,7 +66,7 @@ enum SpiceUsbredirChannelState {
>  };
>
>  struct _SpiceUsbredirChannelPrivate {
> -    libusb_device *device;
> +    SpiceUsbDevice *device;
>      struct usbredirhost *host;
>      /* To catch usbredirhost error messages and report them as a GError */
>      GError **catch_error;
> @@ -211,6 +212,7 @@ static gboolean spice_usbredir_channel_open_device(
>      SpiceUsbredirChannel *channel, GError **err)
>  {
>      SpiceUsbredirChannelPrivate *priv = channel->priv;
> +    SpiceUsbDeviceManager *manager;
>      libusb_device_handle *handle = NULL;
>      int rc, status;
>
> @@ -220,7 +222,11 @@ static gboolean spice_usbredir_channel_open_device(
>  #endif
>                           , FALSE);
>
> -    rc = libusb_open(priv->device, &handle);
> +    manager = spice_usb_device_manager_get(
> +                    spice_channel_get_session(SPICE_CHANNEL(channel)), err);
> +    if (!manager)
> +        return FALSE;
> +    rc = spice_usb_device_libusb_open(manager, priv->device, &handle);
>      if (rc != 0) {
>          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
>                      "Could not open usb device: %s [%i]",
> @@ -236,10 +242,7 @@ static gboolean spice_usbredir_channel_open_device(
>          return FALSE;
>      }
>
> -    if (!spice_usb_device_manager_start_event_listening(
> -            spice_usb_device_manager_get(
> -                spice_channel_get_session(SPICE_CHANNEL(channel)), NULL),
> -            err)) {
> +    if (!spice_usb_device_manager_start_event_listening(manager, err)) {
>          usbredirhost_set_device(priv->host, NULL);
>          return FALSE;
>      }
> @@ -272,7 +275,7 @@ static void spice_usbredir_channel_open_acl_cb(
>      }
>      if (err) {
>          g_simple_async_result_take_error(priv->result, err);
> -        libusb_unref_device(priv->device);
> +        g_object_unref(priv->device);
>          priv->device = NULL;
>          priv->state  = STATE_DISCONNECTED;
>      }
> @@ -290,7 +293,7 @@ static void spice_usbredir_channel_open_acl_cb(
>  G_GNUC_INTERNAL
>  void spice_usbredir_channel_connect_device_async(
>                                            SpiceUsbredirChannel *channel,
> -                                          libusb_device        *device,
> +                                          SpiceUsbDevice       *device,
>                                            GCancellable         *cancellable,
>                                            GAsyncReadyCallback   callback,
>                                            gpointer              user_data)
> @@ -302,7 +305,7 @@ void spice_usbredir_channel_connect_device_async(
>  #endif
>
>      g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
> -    g_return_if_fail(device != NULL);
> +    g_return_if_fail(SPICE_IS_USB_DEVICE(device));
>
>      SPICE_DEBUG("connecting usb channel %p", channel);
>
> @@ -323,7 +326,7 @@ void spice_usbredir_channel_connect_device_async(
>          goto done;
>      }
>
> -    priv->device = libusb_ref_device(device);
> +    priv->device = g_object_ref(device);
>  #if USE_POLKIT
>      priv->result = result;
>      priv->state  = STATE_WAITING_FOR_ACL_HELPER;
> @@ -331,8 +334,8 @@ void spice_usbredir_channel_connect_device_async(
>      g_object_set(spice_channel_get_session(SPICE_CHANNEL(channel)),
>                   "inhibit-keyboard-grab", TRUE, NULL);
>      spice_usb_acl_helper_open_acl(priv->acl_helper,
> -                                  libusb_get_bus_number(device),
> -                                  libusb_get_device_address(device),
> +                                  spice_usb_device_get_busnum(device),
> +                                  spice_usb_device_get_devaddr(device),
>                                    cancellable,
>                                    spice_usbredir_channel_open_acl_cb,
>                                    channel);
> @@ -340,7 +343,7 @@ void spice_usbredir_channel_connect_device_async(
>  #else
>      if (!spice_usbredir_channel_open_device(channel, &err)) {
>          g_simple_async_result_take_error(result, err);
> -        libusb_unref_device(priv->device);
> +        g_object_unref(priv->device);
>          priv->device = NULL;
>      }
>  #endif
> @@ -398,7 +401,7 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
>                  spice_channel_get_session(SPICE_CHANNEL(channel)), NULL));
>          /* This also closes the libusb handle we passed from open_device */
>          usbredirhost_set_device(priv->host, NULL);
> -        libusb_unref_device(priv->device);
> +        g_object_unref(priv->device);
>          priv->device = NULL;
>          priv->state  = STATE_DISCONNECTED;
>          break;
> @@ -406,7 +409,7 @@ void spice_usbredir_channel_disconnect_device(SpiceUsbredirChannel *channel)
>  }
>
>  G_GNUC_INTERNAL
> -libusb_device *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel)
> +SpiceUsbDevice *spice_usbredir_channel_get_device(SpiceUsbredirChannel *channel)
>  {
>      return channel->priv->device;
>  }
> @@ -550,7 +553,7 @@ enum {
>  };
>
>  struct DEVICE_ERROR {
> -    libusb_device *device;
> +    SpiceUsbDevice *device;
>      GError *error;
>  };
>
> @@ -569,7 +572,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;
>      }
> @@ -624,14 +627,13 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
>
>      r = usbredirhost_read_guest_data(priv->host);
>      if (r != 0) {
> -        libusb_device *device = priv->device;
> +        SpiceUsbDevice *device = priv->device;
>          gchar *desc;
>          GError *err;
>
>          g_return_if_fail(device != NULL);
>
> -        desc = spice_usb_device_get_description((SpiceUsbDevice *)device,
> -                                                NULL);
> +        desc = spice_usb_device_get_description(device, NULL);
>          switch (r) {
>          case usbredirhost_read_parse_error:
>              err = g_error_new(SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> @@ -655,9 +657,9 @@ static void usbredir_handle_msg(SpiceChannel *c, SpiceMsgIn *in)
>
>          SPICE_DEBUG("%s", err->message);
>
> -        g_boxed_copy(spice_usb_device_get_type(), device);
> +        g_object_ref(device);
>          emit_main_context(channel, DEVICE_ERROR, device, err);
> -        g_boxed_free(spice_usb_device_get_type(), device);
> +        g_object_unref(device);
>
>          g_error_free(err);
>      }
> diff --git a/gtk/map-file b/gtk/map-file
> index 32ead37..7f18d19 100644
> --- a/gtk/map-file
> +++ b/gtk/map-file
> @@ -100,6 +100,13 @@ spice_usbredir_channel_get_type;
>  spice_util_get_debug;
>  spice_util_get_version_string;
>  spice_util_set_debug;
> +spice_usb_device_get_type;
> +spice_usb_device_new;
> +spice_usb_device_set_info;
> +spice_usb_device_get_busnum;
> +spice_usb_device_get_devaddr;
> +spice_usb_device_get_vid;
> +spice_usb_device_get_pid;
>   
do we need these lines?
>  local:
>  *;
>  };
> diff --git a/gtk/usb-device-manager-priv.h b/gtk/usb-device-manager-priv.h
> index 912e3bf..6da890c 100644
> --- a/gtk/usb-device-manager-priv.h
> +++ b/gtk/usb-device-manager-priv.h
> @@ -34,6 +34,9 @@ void spice_usb_device_manager_stop_event_listening(
>  void spice_usb_device_manager_device_error(
>      SpiceUsbDeviceManager *manager, SpiceUsbDevice *device, GError *err);
>
> +int spice_usb_device_libusb_open(SpiceUsbDeviceManager *self,
> +                                 SpiceUsbDevice *device,
> +                                 struct libusb_device_handle **handle);
>  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 25cb14a..4448a74 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -38,6 +38,8 @@
>  #include "spice-client.h"
>  #include "spice-marshal.h"
>  #include "usb-device-manager-priv.h"
> +#include "spice-usb-device.h"
> +#include "spice-usb-device-priv.h"
>
>  #include <glib/gi18n.h>
>
> @@ -112,14 +114,11 @@ static void spice_usb_device_manager_uevent_cb(GUdevClient     *client,
>                                                 gpointer         user_data);
>  static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
>                                               GUdevDevice            *udev);
> +static
> +libusb_device *spice_usb_device_find_libusb_device(SpiceUsbDeviceManager *self,
> +                                                   SpiceUsbDevice *device);
> +#endif /* ifdef USBREDIR */
>
> -G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
> -                    (GBoxedCopyFunc)libusb_ref_device,
> -                    (GBoxedFreeFunc)libusb_unref_device)
> -
> -#else
> -G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device, g_object_ref, g_object_unref)
> -#endif
>  static void spice_usb_device_manager_initable_iface_init(GInitableIface *iface);
>
>  static guint signals[LAST_SIGNAL] = { 0, };
> @@ -127,6 +126,8 @@ static guint signals[LAST_SIGNAL] = { 0, };
>  G_DEFINE_TYPE_WITH_CODE(SpiceUsbDeviceManager, spice_usb_device_manager, G_TYPE_OBJECT,
>       G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, spice_usb_device_manager_initable_iface_init));
>
> +
> +
>   
nls
>  static void spice_usb_device_manager_init(SpiceUsbDeviceManager *self)
>  {
>      SpiceUsbDeviceManagerPrivate *priv;
> @@ -137,7 +138,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);
> +                                                    g_object_unref);
>  #endif
>  }
>
> @@ -395,7 +396,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_marshal_VOID__BOXED,
>                       G_TYPE_NONE,
>                       1,
> -                     SPICE_TYPE_USB_DEVICE);
> +                     G_TYPE_POINTER);
>   
why are these changes needed?
>      /**
>       * SpiceUsbDeviceManager::device-removed:
> @@ -414,7 +415,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_marshal_VOID__BOXED,
>                       G_TYPE_NONE,
>                       1,
> -                     SPICE_TYPE_USB_DEVICE);
> +                     G_TYPE_POINTER);
>
>      /**
>       * SpiceUsbDeviceManager::auto-connect-failed:
> @@ -435,7 +436,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_user_marshal_VOID__BOXED_BOXED,
>                       G_TYPE_NONE,
>                       2,
> -                     SPICE_TYPE_USB_DEVICE,
> +                     G_TYPE_POINTER,
>                       G_TYPE_ERROR);
>
>      /**
> @@ -457,7 +458,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>                       g_cclosure_user_marshal_VOID__BOXED_BOXED,
>                       G_TYPE_NONE,
>                       2,
> -                     SPICE_TYPE_USB_DEVICE,
> +                     G_TYPE_POINTER,
>                       G_TYPE_ERROR);
>
>      g_type_class_add_private(klass, sizeof(SpiceUsbDeviceManagerPrivate));
> @@ -515,12 +516,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);
>
> @@ -528,16 +529,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);
> +    g_object_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) */
> @@ -562,7 +565,12 @@ 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 = spice_usb_device_new();
> +            spice_usb_device_set_info(device, 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;
>              break;
>          }
>      }
> @@ -579,21 +587,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);
> @@ -604,7 +608,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;
>
> @@ -613,8 +617,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;
>          }
> @@ -727,11 +731,10 @@ void spice_usb_device_manager_device_error(
>  #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++) {
> @@ -796,10 +799,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);
> +                                                  g_object_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, g_object_ref(device));
>      }
>  #endif
>
> @@ -864,7 +867,7 @@ void spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
>              continue; /* Skip already used channels */
>
>          spice_usbredir_channel_connect_device_async(channel,
> -                                 (libusb_device *)device,
> +                                 device,
>                                   cancellable,
>                                   spice_usb_device_manager_channel_connect_cb,
>                                   result);
> @@ -959,13 +962,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_find_libusb_device(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 */
> @@ -1009,34 +1019,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(SPICE_IS_USB_DEVICE(device), 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);
> @@ -1047,3 +1055,75 @@ gchar *spice_usb_device_get_description(SpiceUsbDevice *_device, const gchar *fo
>      return NULL;
>  #endif
>  }
> +
> +
> +
>   
nls
> +/*
> + * 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_find_libusb_device(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(SPICE_IS_USB_DEVICE(device), 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;
> +}
> +
> +int spice_usb_device_libusb_open(SpiceUsbDeviceManager *self,
> +                                 SpiceUsbDevice *device,
> +                                 struct libusb_device_handle **handle)
> +{
> +    libusb_device *ldev;
> +    guint8 bus, addr;
> +    int rc;
> +
> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self), -1);
> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), -2);
> +
> +    bus  = spice_usb_device_get_busnum(device);
> +    addr = spice_usb_device_get_devaddr(device);
> +
> +    ldev = spice_usb_device_find_libusb_device(self, device);
> +    if (ldev) {
> +        rc = libusb_open(ldev, handle);
> +        if (rc) {
> +            const gchar *errstr = spice_usbutil_libusb_strerror(rc);
> +            g_warning("Failed to open usb device %d.%d -- %s (%i)",
> +                      bus, addr, errstr, rc);
> +        }
> +        libusb_unref_device(ldev);
> +    } else {
> +        rc = -3;
>   
doesn't your rc values collide with libusb_error returned by libusb_open?
> +        g_warning("Did not find device %d.%d to open", bus, addr);
> +    }
> +
> +
> +
> +
nls
>     return rc;
> +}
> diff --git a/gtk/usb-device-manager.h b/gtk/usb-device-manager.h
> index df138ee..e5577d8 100644
> --- a/gtk/usb-device-manager.h
> +++ b/gtk/usb-device-manager.h
> @@ -23,6 +23,7 @@
>
>  #include "spice-client.h"
>  #include <gio/gio.h>
> +#include "spice-usb-device.h"
>
>  G_BEGIN_DECLS
>
> @@ -33,13 +34,12 @@ G_BEGIN_DECLS
>  #define SPICE_IS_USB_DEVICE_MANAGER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), SPICE_TYPE_USB_DEVICE_MANAGER))
>  #define SPICE_USB_DEVICE_MANAGER_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), SPICE_TYPE_USB_DEVICE_MANAGER, SpiceUsbDeviceManagerClass))
>
> -#define SPICE_TYPE_USB_DEVICE                    (spice_usb_device_get_type())
> +
>   
nl
>  typedef struct _SpiceUsbDeviceManager SpiceUsbDeviceManager;
>  typedef struct _SpiceUsbDeviceManagerClass SpiceUsbDeviceManagerClass;
>  typedef struct _SpiceUsbDeviceManagerPrivate SpiceUsbDeviceManagerPrivate;
>
> -typedef struct _SpiceUsbDevice SpiceUsbDevice;
>   
nl
>  /**
>   * SpiceUsbDeviceManager:
> @@ -85,7 +85,6 @@ struct _SpiceUsbDeviceManagerClass
>      gchar _spice_reserved[SPICE_RESERVED_PADDING];
>  };
>
> -GType spice_usb_device_get_type(void);
>  GType spice_usb_device_manager_get_type(void);
>
>  gchar *spice_usb_device_get_description(SpiceUsbDevice *device, const gchar *format);
> diff --git a/gtk/usb-device-widget.c b/gtk/usb-device-widget.c
> index 3ed81e4..c342981 100644
> --- a/gtk/usb-device-widget.c
> +++ b/gtk/usb-device-widget.c
> @@ -465,11 +465,6 @@ static void checkbox_clicked_cb(GtkWidget *check, gpointer user_data)
>      spice_usb_device_widget_update_status(self);
>  }
>
> -static void checkbox_usb_device_destroy_notify(gpointer data)
> -{
> -    g_boxed_free(spice_usb_device_get_type(), data);
> -}
> -
>  static void device_added_cb(SpiceUsbDeviceManager *manager,
>      SpiceUsbDevice *device, gpointer user_data)
>  {
> @@ -489,8 +484,8 @@ static void device_added_cb(SpiceUsbDeviceManager *manager,
>
>      g_object_set_data_full(
>              G_OBJECT(check), "usb-device",
> -            g_boxed_copy(spice_usb_device_get_type(), device),
> -            checkbox_usb_device_destroy_notify);
> +            g_object_ref(device),
> +            g_object_unref);
>      g_signal_connect(G_OBJECT(check), "clicked",
>                       G_CALLBACK(checkbox_clicked_cb), self);
>
> diff --git a/spice-common b/spice-common
> index 5f44094..22fc0b0 160000
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 5f4409494066b5f59df58d6207fdbb0441aa9e90
> +Subproject commit 22fc0b0145876b90385c1c88923bcd72a6380812
>
On 06/28/2012 01:09 PM, Christophe Fergeau wrote:
> On Thu, Jun 28, 2012 at 04:46:34AM +0300, Uri Lublin wrote:
>> Note that this change may affect performance a bit, as we now need to look
>> for the libusb_device if we need it. Likely it's negligible.
>> ---
>>   gtk/Makefile.am               |    4 +
>>   gtk/channel-usbredir-priv.h   |    4 +-
>>   gtk/channel-usbredir.c        |   46 +++++-----
>>   gtk/map-file                  |    7 ++
>>   gtk/usb-device-manager-priv.h |    3 +
>>   gtk/usb-device-manager.c      |  194 +++++++++++++++++++++++++++++------------
>>   gtk/usb-device-manager.h      |    5 +-
>>   gtk/usb-device-widget.c       |    9 +--
>>   spice-common                  |    2 +-
>>   9 files changed, 182 insertions(+), 92 deletions(-)
>>
>> @@ -395,7 +396,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>>                        g_cclosure_marshal_VOID__BOXED,
>>                        G_TYPE_NONE,
>>                        1,
>> -                     SPICE_TYPE_USB_DEVICE);
>> +                     G_TYPE_POINTER);
> Why are you not keeping SPICE_TYPE_USB_DEVICE here and in the calls below?

I'll put them back.

>>       /**
>>        * SpiceUsbDeviceManager::device-removed:
>> @@ -414,7 +415,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>>                        g_cclosure_marshal_VOID__BOXED,
>>                        G_TYPE_NONE,
>>                        1,
>> -                     SPICE_TYPE_USB_DEVICE);
>> +                     G_TYPE_POINTER);
>>
>>       /**
>>        * SpiceUsbDeviceManager::auto-connect-failed:
>> @@ -435,7 +436,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>>                        g_cclosure_user_marshal_VOID__BOXED_BOXED,
>>                        G_TYPE_NONE,
>>                        2,
>> -                     SPICE_TYPE_USB_DEVICE,
>> +                     G_TYPE_POINTER,
>>                        G_TYPE_ERROR);
>>
>>       /**
>> @@ -457,7 +458,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
>>                        g_cclosure_user_marshal_VOID__BOXED_BOXED,
>>                        G_TYPE_NONE,
>>                        2,
>> -                     SPICE_TYPE_USB_DEVICE,
>> +                     G_TYPE_POINTER,
>>                        G_TYPE_ERROR);
>>
>>       g_type_class_add_private(klass, sizeof(SpiceUsbDeviceManagerPrivate));
>> @@ -1009,34 +1019,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(SPICE_IS_USB_DEVICE(device), 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);
> I don't think this will do the right thing in error cases, get_vid returns
> an uint16_t which value is 0xffff when there's an error, it will be turned
> into 65535 when it's cast to int, it won't be -1. I haven't looked at the
> rest of the patch for similar issues.

I'll change vid, pid values to 0 on error.


Thanks,
     Uri.
On Thu, Jun 28, 2012 at 03:42:28PM +0300, Uri Lublin wrote:
> On 06/28/2012 01:09 PM, Christophe Fergeau wrote:
> >On Thu, Jun 28, 2012 at 04:46:34AM +0300, Uri Lublin wrote:
> >>+    if ((vid>  0)&&  (pid>  0)) {
> >>+        descriptor = g_strdup_printf("[%04x:%04x]", vid, pid);
> >I don't think this will do the right thing in error cases, get_vid returns
> >an uint16_t which value is 0xffff when there's an error, it will be turned
> >into 65535 when it's cast to int, it won't be -1. I haven't looked at the
> >rest of the patch for similar issues.
> 
> I'll change vid, pid values to 0 on error.

Great, this should solve this issue nicely (assuming 0 is not a valid value
for any of these identifiers).

Christophe