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