[Spice-devel,spice-gtk,Win32,v3,04/12] Add implementation of SpiceUsbDevice as a gobject (new files spice-usb-device*)

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

Details

Message ID 1340848001-7791-5-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.
---
 gtk/spice-usb-device-priv.h |   38 +++++++++++++
 gtk/spice-usb-device.c      |  124 +++++++++++++++++++++++++++++++++++++++++++
 gtk/spice-usb-device.h      |   57 ++++++++++++++++++++
 3 files changed, 219 insertions(+), 0 deletions(-)
 create mode 100644 gtk/spice-usb-device-priv.h
 create mode 100644 gtk/spice-usb-device.c
 create mode 100644 gtk/spice-usb-device.h

Patch hide | download patch | download mbox

diff --git a/gtk/spice-usb-device-priv.h b/gtk/spice-usb-device-priv.h
new file mode 100644
index 0000000..80b3f5c
--- /dev/null
+++ b/gtk/spice-usb-device-priv.h
@@ -0,0 +1,38 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011, 2012 Red Hat, Inc.
+
+   Red Hat Authors:
+   Uri Lublin <uril@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#ifndef __SPICE_USB_DEVICE_PRIV_H__
+#define __SPICE_USB_DEVICE_PRIV_H__
+
+#include <libusb.h>
+
+G_BEGIN_DECLS
+
+SpiceUsbDevice *spice_usb_device_new(void);
+void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev);
+
+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);
+
+G_END_DECLS
+
+#endif /* __SPICE_USB_DEVICE_PRIV_H__ */
diff --git a/gtk/spice-usb-device.c b/gtk/spice-usb-device.c
new file mode 100644
index 0000000..24ac1dd
--- /dev/null
+++ b/gtk/spice-usb-device.c
@@ -0,0 +1,124 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011, 2012 Red Hat, Inc.
+
+   Red Hat Authors:
+   Hans de Goede <hdegoede@redhat.com>
+   Uri Lublin <uril@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "config.h"
+
+#include <glib-object.h>
+#include "spice-usb-device.h"
+#include "spice-usb-device-priv.h"
+#include "usbutil.h"
+#include "glib-compat.h"
+
+#define SPICE_USB_DEVICE_GET_PRIVATE(o) \
+   (G_TYPE_INSTANCE_GET_PRIVATE ((o), SPICE_TYPE_USB_DEVICE, SpiceUsbDevicePrivate))
+
+struct _SpiceUsbDevicePrivate {
+    guint8  bus;
+    guint8  addr;
+    guint16 vid;
+    guint16 pid;
+};
+
+
+G_DEFINE_TYPE(SpiceUsbDevice, spice_usb_device, G_TYPE_OBJECT)
+
+/* ------------------------------------------------------------------ */
+
+static void spice_usb_device_init(SpiceUsbDevice *self)
+{
+    self->priv = SPICE_USB_DEVICE_GET_PRIVATE(self);
+}
+
+static void spice_usb_device_class_init(SpiceUsbDeviceClass *klass)
+{
+    g_type_class_add_private(klass, sizeof(SpiceUsbDevicePrivate));
+}
+
+SpiceUsbDevice *spice_usb_device_new(void)
+{
+    GObject *obj;
+    SpiceUsbDevice *device;
+
+    obj =  g_object_new(SPICE_TYPE_USB_DEVICE, NULL);
+
+    device = SPICE_USB_DEVICE(obj);
+
+    return device;
+}
+
+void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev)
+{
+    SpiceUsbDevicePrivate *priv;
+
+    g_return_if_fail(SPICE_IS_USB_DEVICE(self));
+    priv = self->priv;
+
+    g_warn_if_fail(libdev != NULL);
+
+#ifdef USE_USBREDIR
+    if (libdev) {
+        struct libusb_device_descriptor desc;
+        int errcode;
+        const gchar *errstr;
+
+        priv->bus  = libusb_get_bus_number(libdev);
+        priv->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, priv->bus, priv->addr, errstr, errcode);
+            priv->vid = -1;
+            priv->pid = -1;
+        } else {
+            priv->vid = desc.idVendor;
+            priv->pid = desc.idProduct;
+        }
+    }
+#endif
+}
+
+
+guint8 spice_usb_device_get_busnum(SpiceUsbDevice *device)
+{
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
+    return device->priv->bus;
+}
+
+guint8 spice_usb_device_get_devaddr(SpiceUsbDevice *device)
+{
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
+    return device->priv->addr;
+}
+
+guint16 spice_usb_device_get_vid(SpiceUsbDevice *device)
+{
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
+    return device->priv->vid;
+}
+
+guint16 spice_usb_device_get_pid(SpiceUsbDevice *device)
+{
+    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
+    return device->priv->pid;
+}
diff --git a/gtk/spice-usb-device.h b/gtk/spice-usb-device.h
new file mode 100644
index 0000000..e740e19
--- /dev/null
+++ b/gtk/spice-usb-device.h
@@ -0,0 +1,57 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2011, 2012 Red Hat, Inc.
+
+   Red Hat Authors:
+   Hans de Goede <hdegoede@redhat.com>
+   Uri Lublin <uril@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#ifndef __SPICE_USB_DEVICE_H__
+#define __SPICE_USB_DEVICE_H__
+
+G_BEGIN_DECLS
+
+#define SPICE_TYPE_USB_DEVICE            (spice_usb_device_get_type ())
+#define SPICE_USB_DEVICE(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), SPICE_TYPE_USB_DEVICE, SpiceUsbDevice))
+#define SPICE_USB_DEVICE_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass),  SPICE_TYPE_USB_DEVICE, SpiceUsbDeviceClass))
+#define SPICE_IS_USB_DEVICE(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), SPICE_TYPE_USB_DEVICE))
+#define SPICE_IS_USB_DEVICE_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass),  SPICE_TYPE_USB_DEVICE))
+#define SPICE_USB_DEVICE_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj),  SPICE_TYPE_USB_DEVICE, SpiceUsbDeviceClass))
+
+
+typedef struct _SpiceUsbDevice SpiceUsbDevice;
+typedef struct _SpiceUsbDeviceClass SpiceUsbDeviceClass;
+typedef struct _SpiceUsbDevicePrivate SpiceUsbDevicePrivate;
+
+struct _SpiceUsbDevice
+{
+    GObject parent;
+
+    /*< private >*/
+    SpiceUsbDevicePrivate *priv;
+    /* Do not add fields to this struct */
+};
+
+struct _SpiceUsbDeviceClass
+{
+    GObjectClass parent_class;
+};
+
+GType spice_usb_device_get_type(void);
+
+G_END_DECLS
+
+#endif /* __SPICE_USB_DEVICE_H__ */

Comments

See notes below.
I guess you defined it only for ref counting, otherwise you would have 
used simply a struct (similar to _SpiceUsbDevicePrivate) ?
btw, why define Private and getters and not putting it all public?
Is it the the right gobject-way? no shorter way to do it? (see 
spice_msg_in_ref(SpiceMsgIn *in) in spice-channel.c)

Uri Lublin wrote:
> ---
>  gtk/spice-usb-device-priv.h |   38 +++++++++++++
>  gtk/spice-usb-device.c      |  124 +++++++++++++++++++++++++++++++++++++++++++
>  gtk/spice-usb-device.h      |   57 ++++++++++++++++++++
>  3 files changed, 219 insertions(+), 0 deletions(-)
>  create mode 100644 gtk/spice-usb-device-priv.h
>  create mode 100644 gtk/spice-usb-device.c
>  create mode 100644 gtk/spice-usb-device.h
>
> diff --git a/gtk/spice-usb-device-priv.h b/gtk/spice-usb-device-priv.h
> new file mode 100644
> index 0000000..80b3f5c
> --- /dev/null
> +++ b/gtk/spice-usb-device-priv.h
> @@ -0,0 +1,38 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2011, 2012 Red Hat, Inc.
> +
> +   Red Hat Authors:
> +   Uri Lublin <uril@redhat.com>
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +#ifndef __SPICE_USB_DEVICE_PRIV_H__
> +#define __SPICE_USB_DEVICE_PRIV_H__
> +
> +#include <libusb.h>
> +
> +G_BEGIN_DECLS
> +
> +SpiceUsbDevice *spice_usb_device_new(void);
> +void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev);
> +
> +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);
> +
> +G_END_DECLS
> +
> +#endif /* __SPICE_USB_DEVICE_PRIV_H__ */
> diff --git a/gtk/spice-usb-device.c b/gtk/spice-usb-device.c
> new file mode 100644
> index 0000000..24ac1dd
> --- /dev/null
> +++ b/gtk/spice-usb-device.c
> @@ -0,0 +1,124 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2011, 2012 Red Hat, Inc.
> +
> +   Red Hat Authors:
> +   Hans de Goede <hdegoede@redhat.com>
> +   Uri Lublin <uril@redhat.com>
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include "config.h"
> +
> +#include <glib-object.h>
> +#include "spice-usb-device.h"
> +#include "spice-usb-device-priv.h"
> +#include "usbutil.h"
> +#include "glib-compat.h"
> +
> +#define SPICE_USB_DEVICE_GET_PRIVATE(o) \
> +   (G_TYPE_INSTANCE_GET_PRIVATE ((o), SPICE_TYPE_USB_DEVICE, SpiceUsbDevicePrivate))
> +
> +struct _SpiceUsbDevicePrivate {
> +    guint8  bus;
> +    guint8  addr;
> +    guint16 vid;
> +    guint16 pid;
> +};
> +
> +
> +G_DEFINE_TYPE(SpiceUsbDevice, spice_usb_device, G_TYPE_OBJECT)
> +
> +/* ------------------------------------------------------------------ */
> +
> +static void spice_usb_device_init(SpiceUsbDevice *self)
> +{
> +    self->priv = SPICE_USB_DEVICE_GET_PRIVATE(self);
> +}
> +
> +static void spice_usb_device_class_init(SpiceUsbDeviceClass *klass)
> +{
> +    g_type_class_add_private(klass, sizeof(SpiceUsbDevicePrivate));
> +}
> +
> +SpiceUsbDevice *spice_usb_device_new(void)
> +{
> +    GObject *obj;
> +    SpiceUsbDevice *device;
> +
> +    obj =  g_object_new(SPICE_TYPE_USB_DEVICE, NULL);
> +
>   
why obj is good for?
> +    device = SPICE_USB_DEVICE(obj);
> +
> +    return device;
> +}
> +
> +void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev)
> +{
> +    SpiceUsbDevicePrivate *priv;
> +
> +    g_return_if_fail(SPICE_IS_USB_DEVICE(self));
> +    priv = self->priv;
> +
> +    g_warn_if_fail(libdev != NULL);
> +
> +#ifdef USE_USBREDIR
>   
isn't all of this is used only #ifdef USE_USBREDIR ?
> +    if (libdev) {
> +        struct libusb_device_descriptor desc;
> +        int errcode;
> +        const gchar *errstr;
> +
> +        priv->bus  = libusb_get_bus_number(libdev);
> +        priv->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, priv->bus, priv->addr, errstr, errcode);
> +            priv->vid = -1;
> +            priv->pid = -1;
> +        } else {
> +            priv->vid = desc.idVendor;
> +            priv->pid = desc.idProduct;
> +        }
> +    }
> +#endif
>   
I prefer returning the errorcode.
> +}
> +
> +
>   

Do we really need all the 
g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0) calls when we get 
(SpiceUsbDevice *device) ?
I guess we assume here 0 is illegitimate busnum/devaddr/vid/pid. Is it 
acceptable?
> +guint8 spice_usb_device_get_busnum(SpiceUsbDevice *device)
> +{
> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
> +    return device->priv->bus;
> +}
> +
> +guint8 spice_usb_device_get_devaddr(SpiceUsbDevice *device)
> +{
> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
> +    return device->priv->addr;
> +}
> +
> +guint16 spice_usb_device_get_vid(SpiceUsbDevice *device)
> +{
> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
> +    return device->priv->vid;
> +}
> +
> +guint16 spice_usb_device_get_pid(SpiceUsbDevice *device)
> +{
> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
> +    return device->priv->pid;
> +}
> diff --git a/gtk/spice-usb-device.h b/gtk/spice-usb-device.h
> new file mode 100644
> index 0000000..e740e19
> --- /dev/null
> +++ b/gtk/spice-usb-device.h
> @@ -0,0 +1,57 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2011, 2012 Red Hat, Inc.
> +
> +   Red Hat Authors:
> +   Hans de Goede <hdegoede@redhat.com>
> +   Uri Lublin <uril@redhat.com>
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +#ifndef __SPICE_USB_DEVICE_H__
> +#define __SPICE_USB_DEVICE_H__
> +
> +G_BEGIN_DECLS
> +
> +#define SPICE_TYPE_USB_DEVICE            (spice_usb_device_get_type ())
> +#define SPICE_USB_DEVICE(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), SPICE_TYPE_USB_DEVICE, SpiceUsbDevice))
> +#define SPICE_USB_DEVICE_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass),  SPICE_TYPE_USB_DEVICE, SpiceUsbDeviceClass))
> +#define SPICE_IS_USB_DEVICE(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), SPICE_TYPE_USB_DEVICE))
> +#define SPICE_IS_USB_DEVICE_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass),  SPICE_TYPE_USB_DEVICE))
> +#define SPICE_USB_DEVICE_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj),  SPICE_TYPE_USB_DEVICE, SpiceUsbDeviceClass))
> +
> +
> +typedef struct _SpiceUsbDevice SpiceUsbDevice;
> +typedef struct _SpiceUsbDeviceClass SpiceUsbDeviceClass;
> +typedef struct _SpiceUsbDevicePrivate SpiceUsbDevicePrivate;
> +
> +struct _SpiceUsbDevice
> +{
> +    GObject parent;
> +
> +    /*< private >*/
> +    SpiceUsbDevicePrivate *priv;
> +    /* Do not add fields to this struct */
> +};
> +
> +struct _SpiceUsbDeviceClass
> +{
> +    GObjectClass parent_class;
> +};
> +
> +GType spice_usb_device_get_type(void);
> +
> +G_END_DECLS
> +
> +#endif /* __SPICE_USB_DEVICE_H__ */
>
Hi Arnon,

Thanks for reviewing.

On 06/28/2012 09:45 AM, Arnon Gilboa wrote:
> See notes below.
> I guess you defined it only for ref counting, otherwise you would have 
> used simply a struct (similar to _SpiceUsbDevicePrivate) ?
> btw, why define Private and getters and not putting it all public? 
> Is it the the right gobject-way? no shorter way to do it? (see 
> spice_msg_in_ref(SpiceMsgIn *in) in spice-channel.c) 

You are correct, we need ref-counting (or otherwise copy those structures).
I think using a gobject is better than self-implementing inc_ref, 
dec_ref and destroy.

IIUC, the <class-name>-priv.h file is only included in code inside the 
library,
while <class-name>.h can be included in code outside of the library too.

Another option is to add those calls to usb-device-manager-priv.h

>
> Uri Lublin wrote:
>> ---
>>
>> +
>> +SpiceUsbDevice *spice_usb_device_new(void)
>> +{
>> +    GObject *obj;
>> +    SpiceUsbDevice *device;
>> +
>> +    obj =  g_object_new(SPICE_TYPE_USB_DEVICE, NULL);
>> +
> why obj is good for?

Creating a new object can be done with or without obj.

>> +    device = SPICE_USB_DEVICE(obj);
>> +
>> +    return device;
>> +}
>> +
>> +void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device 
>> *libdev)
>> +{
>> +    SpiceUsbDevicePrivate *priv;
>> +
>> +    g_return_if_fail(SPICE_IS_USB_DEVICE(self));
>> +    priv = self->priv;
>> +
>> +    g_warn_if_fail(libdev != NULL);
>> +
>> +#ifdef USE_USBREDIR
> isn't all of this is used only #ifdef USE_USBREDIR ?

Some is compiled with no USE_USBREDIR.
I think at least the class itself should be defined, but did not try 
without it.

>> +    if (libdev) {
>> +        struct libusb_device_descriptor desc;
>> +        int errcode;
>> +        const gchar *errstr;
>> +
>> +        priv->bus  = libusb_get_bus_number(libdev);
>> +        priv->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, priv->bus, priv->addr, errstr, errcode);
>> +            priv->vid = -1;
>> +            priv->pid = -1;
>> +        } else {
>> +            priv->vid = desc.idVendor;
>> +            priv->pid = desc.idProduct;
>> +        }
>> +    }
>> +#endif
> I prefer returning the errorcode.

Existing code used -1 values here.

>
> Do we really need all the 
> g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0) calls when we get 
> (SpiceUsbDevice *device) ?
> I guess we assume here 0 is illegitimate busnum/devaddr/vid/pid. Is it 
> acceptable?

Previous review comments suggest we do need the g_return_val_if_fail.
I too assume 0 is illegitimate.

>> +guint8 spice_usb_device_get_busnum(SpiceUsbDevice *device)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
>> +    return device->priv->bus;
>> +}
>> +
>> +guint8 spice_usb_device_get_devaddr(SpiceUsbDevice *device)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
>> +    return device->priv->addr;
>> +}
>> +
>> +guint16 spice_usb_device_get_vid(SpiceUsbDevice *device)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
>> +    return device->priv->vid;
>> +}
>> +
>> +guint16 spice_usb_device_get_pid(SpiceUsbDevice *device)
>> +{
>> +    g_return_val_if_fail(SPICE_IS_USB_DEVICE(device), 0);
>> +    return device->priv->pid;
>> +}
On Thu, Jun 28, 2012 at 04:46:33AM +0300, Uri Lublin wrote:
> ---
>  gtk/spice-usb-device-priv.h |   38 +++++++++++++
>  gtk/spice-usb-device.c      |  124 +++++++++++++++++++++++++++++++++++++++++++
>  gtk/spice-usb-device.h      |   57 ++++++++++++++++++++
>  3 files changed, 219 insertions(+), 0 deletions(-)
>  create mode 100644 gtk/spice-usb-device-priv.h
>  create mode 100644 gtk/spice-usb-device.c
>  create mode 100644 gtk/spice-usb-device.h
> 
> diff --git a/gtk/spice-usb-device-priv.h b/gtk/spice-usb-device-priv.h
> new file mode 100644
> index 0000000..80b3f5c
> --- /dev/null
> +++ b/gtk/spice-usb-device-priv.h
> @@ -0,0 +1,38 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2011, 2012 Red Hat, Inc.
> +
> +   Red Hat Authors:
> +   Uri Lublin <uril@redhat.com>
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +#ifndef __SPICE_USB_DEVICE_PRIV_H__
> +#define __SPICE_USB_DEVICE_PRIV_H__
> +
> +#include <libusb.h>
> +
> +G_BEGIN_DECLS
> +
> +SpiceUsbDevice *spice_usb_device_new(void);
> +void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev);
> +
> +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);
> +
> +G_END_DECLS
> +
> +#endif /* __SPICE_USB_DEVICE_PRIV_H__ */
> diff --git a/gtk/spice-usb-device.c b/gtk/spice-usb-device.c
> new file mode 100644
> index 0000000..24ac1dd
> --- /dev/null
> +++ b/gtk/spice-usb-device.c
> @@ -0,0 +1,124 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2011, 2012 Red Hat, Inc.
> +
> +   Red Hat Authors:
> +   Hans de Goede <hdegoede@redhat.com>
> +   Uri Lublin <uril@redhat.com>
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include "config.h"
> +
> +#include <glib-object.h>
> +#include "spice-usb-device.h"
> +#include "spice-usb-device-priv.h"
> +#include "usbutil.h"
> +#include "glib-compat.h"
> +
> +#define SPICE_USB_DEVICE_GET_PRIVATE(o) \
> +   (G_TYPE_INSTANCE_GET_PRIVATE ((o), SPICE_TYPE_USB_DEVICE, SpiceUsbDevicePrivate))
> +
> +struct _SpiceUsbDevicePrivate {
> +    guint8  bus;
> +    guint8  addr;
> +    guint16 vid;
> +    guint16 pid;
> +};
> +
> +
> +G_DEFINE_TYPE(SpiceUsbDevice, spice_usb_device, G_TYPE_OBJECT)
> +
> +/* ------------------------------------------------------------------ */
> +
> +static void spice_usb_device_init(SpiceUsbDevice *self)
> +{
> +    self->priv = SPICE_USB_DEVICE_GET_PRIVATE(self);
> +}
> +
> +static void spice_usb_device_class_init(SpiceUsbDeviceClass *klass)
> +{
> +    g_type_class_add_private(klass, sizeof(SpiceUsbDevicePrivate));
> +}
> +
> +SpiceUsbDevice *spice_usb_device_new(void)
> +{
> +    GObject *obj;
> +    SpiceUsbDevice *device;
> +
> +    obj =  g_object_new(SPICE_TYPE_USB_DEVICE, NULL);
> +
> +    device = SPICE_USB_DEVICE(obj);
> +
> +    return device;
> +}
> +
> +void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev)

When USE_USBREDIR is not defined, there still is a libusb_device definition
available?

> +{
> +    SpiceUsbDevicePrivate *priv;
> +
> +    g_return_if_fail(SPICE_IS_USB_DEVICE(self));
> +    priv = self->priv;
> +
> +    g_warn_if_fail(libdev != NULL);
> +
> +#ifdef USE_USBREDIR
> +    if (libdev) {
> +        struct libusb_device_descriptor desc;
> +        int errcode;
> +        const gchar *errstr;
> +
> +        priv->bus  = libusb_get_bus_number(libdev);
> +        priv->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, priv->bus, priv->addr, errstr, errcode);
> +            priv->vid = -1;
> +            priv->pid = -1;

Won't you get a warning with some compilers if you assign -1 to an unsigned
integer ?

Christophe
On Thu, Jun 28, 2012 at 11:18 AM, Uri Lublin <uril@redhat.com> wrote:
> IIUC, the <class-name>-priv.h file is only included in code inside the
> library,
> while <class-name>.h can be included in code outside of the library too.

Good rule, it's followed more or less in spice-gtk (because everything
is not gobject and some stuffs are legacy).

If SpiceUsbDevice is not exposed at all in the public API, the
gtk/spice-usb-device.h content can be moved to its -priv.h I suppose
On 06/28/2012 01:38 PM, Marc-André Lureau wrote:
> On Thu, Jun 28, 2012 at 11:18 AM, Uri Lublin<uril@redhat.com>  wrote:
>> IIUC, the<class-name>-priv.h file is only included in code inside the
>> library,
>> while<class-name>.h can be included in code outside of the library too.
> Good rule, it's followed more or less in spice-gtk (because everything
> is not gobject and some stuffs are legacy).
>
> If SpiceUsbDevice is not exposed at all in the public API, the
> gtk/spice-usb-device.h content can be moved to its -priv.h I suppose
>

SpiceUsbDevice itself is exposed in public API.
On 06/28/2012 12:49 PM, Christophe Fergeau wrote:
> On Thu, Jun 28, 2012 at 04:46:33AM +0300, Uri Lublin wrote:
>> diff --git a/gtk/spice-usb-device.c b/gtk/spice-usb-device.c
>> +void spice_usb_device_set_info(SpiceUsbDevice *self, libusb_device *libdev)
> When USE_USBREDIR is not defined, there still is a libusb_device definition
> available?

You are correct. This breaks the build with no USE_USBREDIR.
I need to fix that.

>> +{
>> +    SpiceUsbDevicePrivate *priv;
>> +
>> +    g_return_if_fail(SPICE_IS_USB_DEVICE(self));
>> +    priv = self->priv;
>> +
>> +    g_warn_if_fail(libdev != NULL);
>> +
>> +#ifdef USE_USBREDIR
>> +    if (libdev) {
>> +        struct libusb_device_descriptor desc;
>> +        int errcode;
>> +        const gchar *errstr;
>> +
>> +        priv->bus  = libusb_get_bus_number(libdev);
>> +        priv->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, priv->bus, priv->addr, errstr, errcode);
>> +            priv->vid = -1;
>> +            priv->pid = -1;
> Won't you get a warning with some compilers if you assign -1 to an unsigned
> integer ?

I guess there exist compilers that complain about such a statement.
I can remove those lines and leave vid,pid with the value 0.

Thanks,
     Uri.