Hi
Thanks for the helpful comments. See below.
Marc-André Lureau wrote:
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.
sure
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):
right.
I am working on removal of all libusb dependency in win-usb-dev, which
cleanups this mess.
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"
...
ok
+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.
It's used mostly for windows "uevent" generation, so I see no reason for
having multiple instances with their all internal state/allocs etc.
Will use the 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?
As noted above, libusb dependency will be removed.
Thanks for the rest, all will be fixed:
+ 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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel