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