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

[Uri Lublin]

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;
>> +}