[Spice-devel] [spice-gtk Win32 PATCH 3/7] Windows: add win-usb-dev.[ch]: implement GUdevDevice & GUdevClient

[Marc-André Lureau]

Hi

On Mon, May 7, 2012 at 3:15 PM, Uri Lublin <uril at redhat.com> wrote:
> From: Arnon Gilboa <agilboa at redhat.com>
>
> With this patch usb-device-manager can work with little change
> on windows too.
>
> -add uevent signal based on WM_DEVICECHANGE
> -add spice_usb_device_manager_set_window for setting winproc
>   (which is needed for event registration)

You should document any added public API.
Can't we create a private/hidden window for that?

> +WIN_USB_HDRS =                                         \
> +       win-usb-dev.h                                   \
> +       $(NULL)
> +

Perhaps the header doesn't need to be conditionally excluded from the
list of files.
Also if it's not in the public API, it shouldn't be in the
libspice_client_glibinclude_HEADERS.

> +WIN_USB_LIBS = $(GTK_LIBS)

So the spice-glib lib will depend on Gtk? hmm.. I wish we can find
something better.

> @@ -566,6 +586,7 @@ glib_introspection_files =                          \
>        channel-usbredir.c                              \
>        smartcard-manager.c                             \
>        usb-device-manager.c                            \
> +       $(WIN_USB_SRCS)                                 \

Does that file need to be introspected? If it doesn't provide a public
API, I guess not.

> +#ifdef __linux__
>  #include <gudev/gudev.h>
> +#elif WIN32
> +#include "win-usb-dev.h"
> +#endif

We generally prefer the glib defines G_OS_WIN32 and G_OS_UNIX.

In this case USE_GUDEV could also be more appropriate.

> +void spice_usb_device_manager_set_window(SpiceUsbDeviceManager *manager, GdkWindow *win)
> +{
> +#ifdef WIN32
> +    g_udev_client_win_init(manager->priv->udev, manager, win);
> +#endif
> +}

Please document that API, specify when it should be called, and what
kind of window.
Is it safe to be called multiple time, with NULL argument etc.. It
needs to be annotated for introspection bindings to work correctly.

> +gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *usbdev);

Why is this function not static?

> +static GObject *g_udev_client_constructor(GType gtype, guint n_properties,
> +                                          GObjectConstructParam *properties)
> +{
> +    GObject *obj = G_OBJECT_CLASS(g_udev_client_parent_class)->constructor(
> +        gtype, n_properties, properties);
> +    return obj;
> +}

This doesn't need to be overriden.

> +
> +static void g_udev_client_init(GUdevClient *self)
> +{
> +    GUdevClientPrivate *priv;
> +
> +    self->priv = priv = G_UDEV_CLIENT_GET_PRIVATE(self);
> +    priv->usb_dev_mgr = NULL;
> +    priv->dev_list = NULL;
> +    priv->dev_list_size = 0;
> +    priv->udevice_list = NULL;
> +    priv->gdk_win = NULL;

fields are already set to NULL after creation, you can get rid of this
init() override.

> +static void g_udev_client_finalize(GObject *gobject)
> +{
> +    GUdevClient *self = G_UDEV_CLIENT(gobject);
> +    GUdevClientPrivate *priv = self->priv;
> +
> +    if (priv->usb_dev_mgr || priv->gdk_win) {
> +        gdk_window_remove_filter(priv->gdk_win, win_message_cb, priv->usb_dev_mgr);
> +        g_list_free_full(priv->udevice_list, g_free);
> +        libusb_free_device_list(priv->dev_list, 1);
> +        libusb_exit(NULL);
> +    }

This looks prone to leaks if the finalize is called in an object
intermediate state. Instead, it's recommended to check/free each
pointer individually:

if (priv->ptr)
  free_or_unref(priv->ptr)
...

It's also sometime preferable to do all of this in _dispose() and set
the pointers to NULL. This can solves some cyclic references, although
perhaps not necessary here.

What is the effect of calling libusb_exit(NULL); ? (it's already
called by spice_usb_device_manager_finalize)

> +
> +gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *usbdev)
> +{
> +    if (!usbdev) {
> +        return FALSE;
> +    }

There is a convenient function g_return_val_if_fail(usbdev != NULL,
FALSE) for that. It will log if that condition happen too.

> +    if (libusb_get_device_descriptor(dev, &usbdev->desc) < 0) {
> +        return FALSE;
> +    }

It might be worth to add some SPICE_DEBUG?

> +    usbdev->dev = libusb_ref_device(dev);
> +    sprintf(usbdev->sclass, "%d", usbdev->desc.bDeviceClass);
> +    sprintf(usbdev->sbus, "%d", libusb_get_bus_number(dev));
> +    sprintf(usbdev->saddr, "%d", libusb_get_device_address(dev));
> +    return TRUE;

In general snprintf() is preferred.

> +    for (dev = (dev_change > 0 ? devs : priv->dev_list); *dev && !changed_dev; dev++) {
> +        for (d = (dev_change > 0 ? priv->dev_list : devs); *d && *d != *dev; d++);

You could break inside the loop instead of having complex conditions.

> +        if (!*d) {
> +            changed_dev = *dev;
> +        }
> +    }

> +    if (!changed_dev) {
> +        goto leave;
> +    }

if dev_change != 0, shouldn't it also warn it couldn't find any device?

> +    if (dev_change > 0) {
> +        usbdev = g_new(GUdevDeviceInfo, 1);
> +        get_usb_dev_info(changed_dev, usbdev);
> +        udevice = g_udev_device_new(usbdev);
> +        priv->udevice_list = g_list_append(priv->udevice_list, udevice);
> +        SPICE_DEBUG(">>> device inserted: %04x:%04x (bus %s, device %s)\n",
> +                    usbdev->desc.idVendor, usbdev->desc.idProduct, usbdev->sbus, usbdev->saddr);
> +        g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "add", udevice);
> +    } else {
> +        dev_found = FALSE;
> +        for (GList *it = g_list_first(priv->udevice_list); it != NULL && !dev_found; it = g_list_next(it)) {
> +            udevice = (GUdevDevice*)it->data;
> +            usbdev = udevice->priv->usbdev;
> +            dev_found = (usbdev->dev == changed_dev);
> +        }
> +        if (!dev_found) {
> +            goto leave;
> +        }
> +        SPICE_DEBUG("<<< device removed: %04x:%04x (bus %s, device %s)\n",
> +                    usbdev->desc.idVendor, usbdev->desc.idProduct, usbdev->sbus, usbdev->saddr);
> +        g_signal_emit(self, signals[UEVENT_SIGNAL], 0, "remove", udevice);
> +        priv->udevice_list = g_list_remove(priv->udevice_list, udevice);
> +        g_object_unref(udevice);
> +    }

> +    if (priv->usb_dev_mgr || priv->gdk_win || !manager || !win) {
> +        return FALSE;
> +    }

g_return_val_if_fail on each condition (one line for each, so we can
separate failure for each condition if it occurs)


> +static GUdevDevice *g_udev_device_new(GUdevDeviceInfo *usbdev)
> +{

Could be worth adding g_return_val_if_fail (usbdev != NULL, NULL)

> +    GUdevDevice *device =  G_UDEV_DEVICE(g_object_new(G_UDEV_TYPE_DEVICE, NULL));
> +
> +    device->priv->usbdev = usbdev;
> +    return device;
> +}


> +const gchar *g_udev_device_get_property(GUdevDevice *udev, const gchar *property)
> +{
> +    GUdevDeviceInfo* usbdev = udev->priv->usbdev;
> +
Checking argument could be worth:
g_return_val_if_fail(G_UDEV_IS_DEVICE(dev)..)
g_return_val_if_fail(property != NULL)

> +    if (strcmp(property, "BUSNUM") == 0) {

We generally prefer g_strcmp0() , a bit safer.

> +        return usbdev->sbus;
> +    } else if (strcmp(property, "DEVNUM") == 0) {
> +        return usbdev->saddr;
> +    } else if (strcmp(property, "DEVTYPE") == 0) {
> +        return "usb_device";
> +    }

If the value is different it would be worth to warn with
g_warn_if_reached() for example.

> +const gchar *g_udev_device_get_sysfs_attr(GUdevDevice *udev, const gchar *attr)
> +{

same comments as previous function


-- 
Marc-André Lureau