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

[Marc-André Lureau]

Hi

> +WIN_USB_FILES= \
> +       win-usb-dev.h                   \
> +       win-usb-dev.c                   \
> +       usbclerk.h                      \
> +       $(NULL)
> +
> +if OS_WIN32
> +libspice_client_glib_2_0_la_SOURCES += \
> +       $(WIN_USB_FILES)
> +else
> +EXTRA_DIST += $(WIN_USB_FILES)
> +endif

usbclerk.h is missing from this patch. It might make sense to call it
win-usb-clerk.h for consistency.

No need for EXTRA_DIST here, automake takes care of that even inside
conditionals.

Since win-usb-dev.* includes and use heavily libusb, it might make
sense to add WITH_USBREDIR condition (otherwise build fails there if
libusb is missing):

if OS_WIN32
if WITH_USBREDIR
 libspice_client_glib_2_0_la_SOURCES += \
        win-usb-dev.h                   \
        win-usb-dev.c                   \
        usbclerk.h                      \
        $(NULL)
endif
endif

> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index 14b60c9..2b6ce28 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -28,7 +28,14 @@
>  #ifdef USE_USBREDIR
>  #include <errno.h>
>  #include <libusb.h>
> +#if defined(USE_GUDEV)
>  #include <gudev/gudev.h>
> +#elif defined(G_OS_WIN32)
> +#include "win-usb-dev.h"
> +#else
> +#warning "Expecting one of G_OS_WIN32 and USE_GUDEV to be defined"
> +#endif

adding a few spaces for indentation could be helpful for readibility,
something like:

# if defined(USE_GUDEV)
#  include <gudev/gudev.h>
# elif defined(G_OS_WIN32)
#  include "win-usb-dev.h"
...

> +static GUdevClient *singletone = NULL;
...
> +GUdevClient *g_udev_client_new(const gchar* const *subsystems)
> +{
> +    if (!singletone) {
> +        singletone = g_initable_new(G_UDEV_TYPE_CLIENT, NULL, NULL, NULL);
> +        return singletone;
> +    } else {
> +        return g_object_ref(singletone);
> +    }
> +}

Why it has to be a singleton? Is this a Windows limitation? Or is it a
kind of optimization? Gudev doesn't seem to return a singleton. Also,
if it can't be a regular life object, there is a safer way of
allocating it, as shown in the smartcard-manager using GOnce.

> +    /* FIXME: Using context != NULL results in a crash on spice_usb_device_manager_add_dev().
> +     * The same happens also when using SpiceUsbDeviceManagerPrivate->context and
> +     * removing the calls to libusb_init & libusb_exit.
> +     */
> +    rc = libusb_init(NULL);
> +    g_return_val_if_fail(rc >= 0, FALSE);

Could it be because libusb_init is also called from
usb-device-manager.c? Should it be consolidated somehow?

> +    priv->dev_list_size = libusb_get_device_list(NULL, &priv->dev_list);
> +    g_return_val_if_fail(priv->dev_list_size >= 0, FALSE);

I would add g_return_val_if_fail(priv->dev_list != NULL, FALSE);

> +    for (dev = priv->dev_list; *dev; dev++) {

I would add g_return_val_if_fail(dev != NULL, FALSE)

> +        usbdev = g_new(GUdevDeviceInfo, 1);

it might be safer to use g_new0() (this is a rule of thumb to allow
easier debugging later on)

> +GList *g_udev_client_query_by_subsystem(GUdevClient *self, const gchar *subsystem)
> +{

We should check given arguments:

g_return_val_if_fail(G_IS_UDEV_CLIENT(self), NULL);
g_return_val_if_fail(g_strncmp0(subsystem, "usb") == 0, NULL);


> +    if (dev_change > 0) {
> +        usbdev = g_new(GUdevDeviceInfo, 1);

g_new0()

> +        get_usb_dev_info(changed_dev, usbdev);
> +        udevice = g_udev_device_new(usbdev);

Wouldn't it make sense to have a the GudevDevice ctor doing all this?
You would call  g_udev_device_new(libusb_device), and it would
allocate device_info and call get_usb_dev_info internally.

-- 
Marc-André Lureau