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;
+}
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel