[Spice-devel] [spice-gtk Win32 v3 07/12] Windows mingw: usb: implement GUdevDevice & GUdevClient for windows

[Uri Lublin]

On 06/28/2012 04:36 PM, Arnon Gilboa wrote:
> reviewed most of it before;) but added some comments for your changes 
> below.
>
> Uri Lublin wrote:
>> From: Arnon Gilboa <agilboa at redhat.com>
>>
>>
>> +struct _GUdevDevicePrivate
>> +{
>> +    /* FixMe: move above fields to this structure and access them 
>> directly */
> it will cleanup some code as well, remove the g_new0/g_free calls.

Agreed. Disclaimer: dropping GUdevDeviceInfo is Arnon's idea.

>
> you will also find it nice to merge get_usb_dev_info into 
> g_udev_device_new, so it will be
>
> static GUdevDevice *g_udev_device_new(libusb_device *dev, 
> GUdevDeviceInfo *udevinfo);

Without the udevinfo.


>>
>> +/*
>> + * devs [in,out] an empty devs list in, full devs list out
>> + * Returns: number-of-devices, or a negative value on error.
>> + */
>> +static ssize_t
>> +g_udev_client_list_devices(GUdevClient *self, GList **devs,
>> +                           GError **err, const gchar *name)
>> +{
> is it really required to have err & name params?

It's not required.

>> +    gssize rc;
>> +    libusb_device **lusb_list, **dev;
>> +    GUdevClientPrivate *priv;
>> +    GUdevDeviceInfo *udevinfo;
>> +    GUdevDevice *udevice;
>> +    ssize_t n;
>> +
>> +    g_return_val_if_fail(G_UDEV_IS_CLIENT(self), -1);
>> +    g_return_val_if_fail(devs != NULL, -2);
>> +
>> +    priv = self->priv;
>> +
>> +    g_return_val_if_fail(self->priv->ctx != NULL, -3);
>> +
>> +    rc = libusb_get_device_list(priv->ctx, &lusb_list);
>> +    if (rc < 0) {
>> +        const char *errstr = spice_usbutil_libusb_strerror(rc);
>> +        g_warning("%s: libusb_get_device_list failed", name);
>> +        g_set_error(err, G_UDEV_CLIENT_ERROR, 
>> G_UDEV_CLIENT_LIBUSB_FAILED,
>> +                    "%s: Error getting device list from libusb: %s 
>> [%i]",
>> +                    name, errstr, rc);
>> +        return -4;
>> +    }
>> +
>> +    n = rc;
>> +
>> +    for (dev = lusb_list; *dev; dev++) {
>> +        udevinfo = g_new0(GUdevDeviceInfo, 1);
>> +        get_usb_dev_info(*dev, udevinfo);
>> +        udevice = g_udev_device_new(udevinfo);
>> +        *devs = g_list_prepend(*devs, udevice);
>> +    }
>> +    libusb_free_device_list(lusb_list, 1);
>> +
>> +    return n;
>> +}
> I personally don't like these return codes.
>> +
>> +static void g_udev_client_free_device_list(GList **devs)
>> +{
>> +    g_return_if_fail(devs != NULL);
>> +    if (*devs) {
>> +        g_list_free_full(*devs, g_object_unref);
>> +        *devs = NULL;
>> +    }
>> +}
>> +
>> +

>> +
>> +static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo 
>> *udevinfo)
>> +{
>> +    struct libusb_device_descriptor desc;
>> +
>> +    g_return_val_if_fail(dev, FALSE);
>> +    g_return_val_if_fail(udevinfo, FALSE);
>> +
>> +    if (libusb_get_device_descriptor(dev, &desc) < 0) {
>> +        g_warning("cannot get device descriptor %p", dev);
>> +        return FALSE;
>> +    }
>> +
>> +    udevinfo->bus   = libusb_get_bus_number(dev);
>> +    udevinfo->addr  = libusb_get_device_address(dev);
>> +    udevinfo->class = desc.bDeviceClass;
>> +    udevinfo->vid   = desc.idVendor;
>> +    udevinfo->pid   = desc.idProduct;
>> +    snprintf(udevinfo->sclass, sizeof(udevinfo->sclass), "%d", 
>> udevinfo->class);
>> +    snprintf(udevinfo->sbus,   sizeof(udevinfo->sbus),   "%d", 
>> udevinfo->bus);
>> +    snprintf(udevinfo->saddr,  sizeof(udevinfo->saddr),  "%d", 
>> udevinfo->addr);
>> +    return TRUE;
>> +}
>> +
>> +#define INT_RETURN_IF_DIFFERENT(x, y) \
>> +    do { gint c,d; c=(x); d=(y); if(c-d) return (c-d); } while(0)
>> +
> funny macro;) I prefer removing it and simply using if (a - b) return 
> FALSE;

You mean if (a-b) return (a-b);
That's what the macro does, except it first cast  x,y  to gint type.

>> +static gint gudev_device_compare(GUdevDevice *a, GUdevDevice *b)
> gboolean return might be nicer

Compare usually returns an int (<0, =0 , >0 values).
This makes the function usable by sort.
(I thought of sorting and break out of the loop sooner, but I think it's not
worth it, as the expected list length is pretty small).

>> +{
>> +    GUdevDeviceInfo *ai, *bi;
>> +
>> +    g_return_val_if_fail(G_UDEV_DEVICE(a), 1);
>> +    g_return_val_if_fail(G_UDEV_DEVICE(b), -1);
>> +
>> +    ai = a->priv->udevinfo;
>> +    bi = b->priv->udevinfo;
>> +
>> +    INT_RETURN_IF_DIFFERENT(ai->bus, bi->bus);
>> +    INT_RETURN_IF_DIFFERENT(ai->addr, bi->addr);
>> +    INT_RETURN_IF_DIFFERENT(ai->vid, bi->vid);
>> +    INT_RETURN_IF_DIFFERENT(ai->pid, bi->pid);
>> +
>> +    return 0;
> return TRUE;

That works too, but needs to rename the function (Equal?).

>> +    if (is_dev_change > 0) {
>> +        llist  = now_devs;
>> +        slist = priv->udev_list;
>> +    } else {
>> +        llist = priv->udev_list;
>> +        slist  = now_devs;
>> +    }
>> +
>> +    g_udev_device_print_list(llist, "handle_dev_change: long list:");
>> +    g_udev_device_print_list(slist, "handle_dev_change: short list:");
> do we still need these debug lines?

Only if we want to debug the lists. In that case one would
have to define DEBUG_GUDEV_DEVICE_LISTS above.
When DEBUG_GUDEV_DEVICE_LISTS is not defined these lines are empty.

>
>> +
>> +#ifdef DEBUG_GUDEV_DEVICE_LISTS
>> +static void g_udev_device_print_list(GList *l, const gchar *msg)
>> +{
>> +    GList *it;
>> +
>> +    for (it = g_list_first(l); it != NULL; it=g_list_next(it)) {
>> +        g_udev_device_print(it->data, msg);
>> +    }
>> +}
>> +#endif
> still need it?

Answered above.

>> diff --git a/gtk/win-usb-dev.h b/gtk/win-usb-dev.h
>> +
>> +typedef struct _SpiceUsbDeviceManager SpiceUsbDeviceManager;
> any reason we need to know SpiceUsbDeviceManager here?

No. I'll remove it.


Thanks,
     Uri.