[Spice-devel,spice-gtk,Win32,v4,02/17] Introduce SpiceUsbDeviceInfo to be kept instead of a libusb_device

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

Details

Message ID 1341521049-4562-3-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.
For Windows, it's better not to keep references for libusb_devices
that are not used.
So instead of makeing SpiceUsbDevice a box for a libusb_device
it is going to be a box for a SpiceUsbDeviceInfo.
---
 gtk/usb-device-manager.c |  145 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 145 insertions(+), 0 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
index b39c2d4..c4cbc20 100644
--- a/gtk/usb-device-manager.c
+++ b/gtk/usb-device-manager.c
@@ -101,6 +101,16 @@  struct _SpiceUsbDeviceManagerPrivate {
     GPtrArray *channels;
 };

+typedef struct _SpiceUsbDeviceInfo {
+#ifdef USE_USBREDIR
+    guint8  busnum;
+    guint8  devaddr;
+    guint16 vid;
+    guint16 pid;
+    int     ref;
+#endif
+} SpiceUsbDeviceInfo;
+
 #ifdef USE_USBREDIR
 static void channel_new(SpiceSession *session, SpiceChannel *channel,
                         gpointer user_data);
@@ -113,6 +123,12 @@  static void spice_usb_device_manager_uevent_cb(GUdevClient     *client,
 static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager  *self,
                                              GUdevDevice            *udev);

+static SpiceUsbDeviceInfo *spice_usb_device_set_info(libusb_device *libdev);
+static SpiceUsbDevice *spice_usb_device_ref(SpiceUsbDevice *device);
+static void spice_usb_device_unref(SpiceUsbDevice *device);
+
+static gboolean spice_usb_device_equal_libdev(SpiceUsbDevice *device,
+                                              libusb_device *libdev);
 G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
                     (GBoxedCopyFunc)libusb_ref_device,
                     (GBoxedFreeFunc)libusb_unref_device)
@@ -1052,3 +1068,132 @@  gchar *spice_usb_device_get_description(SpiceUsbDevice *_device, const gchar *fo
     return NULL;
 #endif
 }
+
+
+
+#ifdef USE_USBREDIR
+/*
+ * SpiceUsbDeviceInfo
+ */
+#ifdef __GNUC__ /* current implementation requires gcc */
+static int atomic_inc(int *a)
+{
+    return __sync_add_and_fetch (a, 1);
+}
+static int atomic_dec(int *a)
+{
+    return __sync_sub_and_fetch (a, 1);
+}
+#else /* not __GNUC__ ( and not atomic) */
+#error "atomic_inc and a atomic dec are only implemented for GNUC"
+#endif /* __GNUC__ */
+
+static SpiceUsbDeviceInfo *spice_usb_device_set_info(libusb_device *libdev)
+{
+    SpiceUsbDeviceInfo *info;
+    struct libusb_device_descriptor desc;
+    int errcode;
+    const gchar *errstr;
+    guint8 bus, addr;
+
+    g_return_val_if_fail(libdev != NULL, NULL);
+
+    bus = libusb_get_bus_number(libdev);
+    addr = libusb_get_device_address(libdev);
+
+    errcode = libusb_get_device_descriptor(libdev, &desc);
+    if (errcode < 0) {
+        errstr = spice_usbutil_libusb_strerror(errcode);
+        g_warning("cannot get device descriptor for (%p) %d.%d -- %s(%d)",
+                  libdev, bus, addr, errstr, errcode);
+        return NULL;
+    }
+
+    info = g_new0(SpiceUsbDeviceInfo, 1);
+
+    info->busnum  = bus;
+    info->devaddr = addr;
+    info->vid = desc.idVendor;
+    info->pid = desc.idProduct;
+    info->ref = 1;
+
+    return info;
+}
+
+guint8 spice_usb_device_get_busnum(SpiceUsbDevice *device)
+{
+    SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
+
+    g_return_val_if_fail(info != NULL, 0);
+
+    return info->busnum;
+}
+
+guint8 spice_usb_device_get_devaddr(SpiceUsbDevice *device)
+{
+    SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
+
+    g_return_val_if_fail(info != NULL, 0);
+
+    return info->devaddr;
+}
+
+guint16 spice_usb_device_get_vid(SpiceUsbDevice *device)
+{
+    SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
+
+    g_return_val_if_fail(info != NULL, 0);
+
+    return info->vid;
+}
+
+guint16 spice_usb_device_get_pid(SpiceUsbDevice *device)
+{
+    SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
+
+    g_return_val_if_fail(info != NULL, 0);
+
+    return info->pid;
+}
+
+static SpiceUsbDevice *spice_usb_device_ref(SpiceUsbDevice *device)
+{
+    SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
+
+    g_return_val_if_fail(info != NULL, NULL);
+    atomic_inc(&info->ref);
+    return device;
+}
+
+static void spice_usb_device_unref(SpiceUsbDevice *device)
+{
+    int ref;
+
+    SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
+
+    g_return_if_fail(info != NULL);
+
+    ref = atomic_dec(&info->ref);
+    if (ref == 0) {
+        memset(info, 0, sizeof(*info));
+        g_free(info);
+    }
+}
+
+static gboolean
+spice_usb_device_equal_libdev(SpiceUsbDevice *device,
+                              libusb_device  *libdev)
+{
+    guint8 addr1, addr2, bus1, bus2;
+
+    if ((device == NULL) || (libdev == NULL))
+        return FALSE;
+
+    bus1  = spice_usb_device_get_busnum(device);
+    addr1 = spice_usb_device_get_devaddr(device);
+    bus2  = libusb_get_bus_number(libdev);
+    addr2 = libusb_get_device_address(libdev);
+
+    return ((bus1 == bus2) && (addr1 == addr2));
+}
+#endif /* USE_USBREDIR */

Comments

On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin <uril@redhat.com> wrote:
> +    ref = atomic_dec(&info->ref);
> +    if (ref == 0) {
> +        memset(info, 0, sizeof(*info));
> +        g_free(info);
> +    }

we should really be using the g_atomic operations.
g_atomic_int_dec_and_test () is idiomatic for unref (it may be more
correct). the memset isn't really helping here.
Hi,

On 07/05/2012 10:43 PM, Uri Lublin wrote:
> For Windows, it's better not to keep references for libusb_devices
> that are not used.
> So instead of makeing SpiceUsbDevice a box for a libusb_device
> it is going to be a box for a SpiceUsbDeviceInfo.
> ---
>   gtk/usb-device-manager.c |  145 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 145 insertions(+), 0 deletions(-)
>
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index b39c2d4..c4cbc20 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -101,6 +101,16 @@ struct _SpiceUsbDeviceManagerPrivate {
>       GPtrArray *channels;
>   };
>
> +typedef struct _SpiceUsbDeviceInfo {
> +#ifdef USE_USBREDIR
> +    guint8  busnum;
> +    guint8  devaddr;
> +    guint16 vid;
> +    guint16 pid;
> +    int     ref;
> +#endif
> +} SpiceUsbDeviceInfo;
> +
>   #ifdef USE_USBREDIR
>   static void channel_new(SpiceSession *session, SpiceChannel *channel,
>                           gpointer user_data);

Many compilers barf on empty structs, at a minimum you need to put an "int dummy;" in there.

+What Marc-André said :)
Hi,

Looking at the next patch in the series, I've one more remark on this one:

On 07/05/2012 10:43 PM, Uri Lublin wrote:
> +static SpiceUsbDeviceInfo *spice_usb_device_set_info(libusb_device *libdev)
> +{
> +    SpiceUsbDeviceInfo *info;
> +    struct libusb_device_descriptor desc;
> +    int errcode;
> +    const gchar *errstr;
> +    guint8 bus, addr;
> +
> +    g_return_val_if_fail(libdev != NULL, NULL);
> +
> +    bus = libusb_get_bus_number(libdev);
> +    addr = libusb_get_device_address(libdev);
> +
> +    errcode = libusb_get_device_descriptor(libdev, &desc);
> +    if (errcode < 0) {
> +        errstr = spice_usbutil_libusb_strerror(errcode);
> +        g_warning("cannot get device descriptor for (%p) %d.%d -- %s(%d)",
> +                  libdev, bus, addr, errstr, errcode);
> +        return NULL;
> +    }
> +
> +    info = g_new0(SpiceUsbDeviceInfo, 1);
> +
> +    info->busnum  = bus;
> +    info->devaddr = addr;
> +    info->vid = desc.idVendor;
> +    info->pid = desc.idProduct;
> +    info->ref = 1;
> +
> +    return info;
> +}

This function creates a new SpiceUsbDevice from the info in a libusb_device, so its
name is sort of confusing since it is not setting the info of the passed in object, but
creating a new object, I would like to see this renamed to: spice_usb_device_new

Regards,

Hans
On 07/06/2012 12:21 AM, Marc-André Lureau wrote:
> On Thu, Jul 5, 2012 at 10:43 PM, Uri Lublin<uril@redhat.com>  wrote:
>> +    ref = atomic_dec(&info->ref);
>> +    if (ref == 0) {
>> +        memset(info, 0, sizeof(*info));
>> +        g_free(info);
>> +    }
> we should really be using the g_atomic operations.
> g_atomic_int_dec_and_test () is idiomatic for unref (it may be more
> correct). the memset isn't really helping here.
>

done.
The memset was there to catch memory problems (bugs). I dropped it.

Thanks,
     Uri.
On 07/06/2012 09:26 AM, Hans de Goede wrote:
> On 07/05/2012 10:43 PM, Uri Lublin wrote:
>>
>> +typedef struct _SpiceUsbDeviceInfo {
>> +#ifdef USE_USBREDIR
>> +    guint8  busnum;
>> +    guint8  devaddr;
>> +    guint16 vid;
>> +    guint16 pid;
>> +    int     ref;
>> +#endif
>> +} SpiceUsbDeviceInfo;
>> +
>>   #ifdef USE_USBREDIR
>>   static void channel_new(SpiceSession *session, SpiceChannel *channel,
>>                           gpointer user_data);
>
> Many compilers barf on empty structs, at a minimum you need to put an 
> "int dummy;" in there.
>

Hi Hans,

I moved the whole struct below the next #ifdef USE_USBREDIR.

Thanks,
     Uri.
On 07/06/2012 09:30 AM, Hans de Goede wrote:
> Looking at the next patch in the series, I've one more remark on this 
> one:
>
> On 07/05/2012 10:43 PM, Uri Lublin wrote:
>> +static SpiceUsbDeviceInfo *spice_usb_device_set_info(libusb_device 
>> *libdev)
>> +{
>> +    SpiceUsbDeviceInfo *info;
>> +    struct libusb_device_descriptor desc;
>> +    int errcode;
>> +    const gchar *errstr;
>> +    guint8 bus, addr;
>> +
>> +    g_return_val_if_fail(libdev != NULL, NULL);
>> +
>> +    bus = libusb_get_bus_number(libdev);
>> +    addr = libusb_get_device_address(libdev);
>> +
>> +    errcode = libusb_get_device_descriptor(libdev, &desc);
>> +    if (errcode < 0) {
>> +        errstr = spice_usbutil_libusb_strerror(errcode);
>> +        g_warning("cannot get device descriptor for (%p) %d.%d -- 
>> %s(%d)",
>> +                  libdev, bus, addr, errstr, errcode);
>> +        return NULL;
>> +    }
>> +
>> +    info = g_new0(SpiceUsbDeviceInfo, 1);
>> +
>> +    info->busnum  = bus;
>> +    info->devaddr = addr;
>> +    info->vid = desc.idVendor;
>> +    info->pid = desc.idProduct;
>> +    info->ref = 1;
>> +
>> +    return info;
>> +}
>
> This function creates a new SpiceUsbDevice from the info in a 
> libusb_device, so its
> name is sort of confusing since it is not setting the info of the 
> passed in object, but
> creating a new object, I would like to see this renamed to: 
> spice_usb_device_new
>

done.

Thanks,
     Uri.