Hey,
Sorry, it took a bit of time to review, but this patch is sticking a lot
of changes together, splitting such patches in multiple smaller ones
really help to get speedier reviews (and actually, probably improves
these reviews, there are some things I would have missed if I did not
split this locally myself).
On Mon, Feb 11, 2019 at 11:42:03AM +0200, Yuri Benditovich wrote:
> Discard unneeded GUDev simulation and use persistent libusb
> device pointer in SpiceUsbDevice on Windows as we do on Linux.
> This removes significant part of differences between Linux and
> Windows code. GUDevClient in win-usb-dev.c now maintains list
> of libusb_device pointers and passes new and removed libusb devices
> to usb-device-manager via signal.
Yes, I would even emphazise in the commit log that this is the *only*
thing GUdevClient does now, it's just a wrapper on top of libusb which
gives us hotplug events on Windows.
What this commit does in addition to using a persistent libusb device
pointer is to kill GUdevInfo and GUdevDevice, which means some
s/GUdevDevice/libusb_device in the codebase. It also changes some
USE_GUDEV to G_OS_WIN32, remove an unused field in SpiceUsbDeviceInfo,
changes callback parameters from "add"/"remove" to a boolean,
... (and that really is a lot to do in a single patch:-/ )
> Important part of the change is that usb-device-manager on Windows
> does not create its own libusb context and uses one created by
> win-usb-dev (as libusb device must be enumerated and redirected
> with the same libusb context).
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx>
> ---
> src/usb-device-manager.c | 290 +++++++--------------------------------
> src/win-usb-dev.c | 254 ++++++++++------------------------
> src/win-usb-dev.h | 34 +----
> 3 files changed, 126 insertions(+), 452 deletions(-)
>
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 55b672b..009b15b 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -33,7 +33,6 @@
>
> #if defined(G_OS_WIN32)
> #include "win-usb-dev.h"
> -#define USE_GUDEV /* win-usb-dev.h provides a fake gudev interface */
> #endif
>
> #include "channel-usbredir-priv.h"
> @@ -106,11 +105,10 @@ struct _SpiceUsbDeviceManagerPrivate {
> struct usbredirfilter_rule *redirect_on_connect_rules;
> int auto_conn_filter_rules_count;
> int redirect_on_connect_rules_count;
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> GUdevClient *udev;
> - libusb_device **coldplug_list; /* Avoid needless reprobing during init */
> #else
> - gboolean redirecting; /* Handled by GUdevClient in the gudev case */
> + gboolean redirecting; /* Handled by GUdevClient in Windows */
> libusb_hotplug_callback_handle hp_handle;
> #endif
> #ifdef G_OS_WIN32
> @@ -141,11 +139,7 @@ typedef struct _SpiceUsbDeviceInfo {
> guint16 vid;
> guint16 pid;
> gboolean isochronous;
> -#ifdef G_OS_WIN32
> - guint8 state;
> -#else
> libusb_device *libdev;
> -#endif
> gint ref;
> } SpiceUsbDeviceInfo;
>
> @@ -156,13 +150,11 @@ static void channel_destroy(SpiceSession *session, SpiceChannel *channel,
> gpointer user_data);
> static void channel_event(SpiceChannel *channel, SpiceChannelEvent event,
> gpointer user_data);
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> - const gchar *action,
> - GUdevDevice *udevice,
> + libusb_device *libdev,
> + gboolean add,
> gpointer user_data);
> -static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> - GUdevDevice *udev);
> #else
> static int spice_usb_device_manager_hotplug_cb(libusb_context *ctx,
> libusb_device *device,
> @@ -210,7 +202,7 @@ G_DEFINE_BOXED_TYPE(SpiceUsbDevice, spice_usb_device,
> static void
> _set_redirecting(SpiceUsbDeviceManager *self, gboolean is_redirecting)
> {
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> g_object_set(self->priv->udev, "redirecting", is_redirecting, NULL);
> #else
> self->priv->redirecting = is_redirecting;
> @@ -235,7 +227,7 @@ gboolean spice_usb_device_manager_is_redirecting(SpiceUsbDeviceManager *self)
> {
> #ifdef USE_USBREDIR
>
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> gboolean redirecting;
> g_object_get(self->priv->udev, "redirecting", &redirecting, NULL);
> return redirecting;
> @@ -286,11 +278,20 @@ static gboolean spice_usb_device_manager_initable_init(GInitable *initable,
> SpiceUsbDeviceManagerPrivate *priv = self->priv;
> GList *list;
> GList *it;
> - int rc;
> -#ifdef USE_GUDEV
> - const gchar *const subsystems[] = {"usb", NULL};
> -#endif
>
> + /* Start listening for usb devices plug / unplug */
> +#ifdef G_OS_WIN32
> + /* libusb_init called by win-usb-dev */
> + priv->udev = g_udev_client_new(&priv->context);
> + if (priv->udev == NULL) {
> + g_warning("Error initializing GUdevClient");
> + return FALSE;
> + }
> + g_signal_connect(G_OBJECT(priv->udev), "uevent",
> + G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
> + g_udev_client_report_devices(priv->udev);
g_udev_client_new will have registered its wcls.lpfnWndProc once it
returns, I assume its callback won't be called before the mainloop runs?
I'm worried if we can get a race with a notification happening between
g_udev_client_new and g_udev_client_report_devices, which would cause
the same device to be signaled twice I think.
There was a /* Do coldplug (detection of already connected devices) */
comment before, I would keep it before the call to
g_udev_client_report_devices().
> +#else
> + int rc;
> /* Initialize libusb */
> rc = libusb_init(&priv->context);
> if (rc < 0) {
> @@ -300,33 +301,6 @@ static gboolean spice_usb_device_manager_initable_init(GInitable *initable,
> "Error initializing USB support: %s [%i]", desc, rc);
> return FALSE;
> }
> -
> -#ifdef G_OS_WIN32
> -#if LIBUSB_API_VERSION >= 0x01000106
> - libusb_set_option(priv->context, LIBUSB_OPTION_USE_USBDK);
> -#endif
> -#endif
> -
> - /* Start listening for usb devices plug / unplug */
> -#ifdef USE_GUDEV
> - priv->udev = g_udev_client_new(subsystems);
> - if (priv->udev == NULL) {
> - g_warning("Error initializing GUdevClient");
> - return FALSE;
> - }
> - g_signal_connect(G_OBJECT(priv->udev), "uevent",
> - G_CALLBACK(spice_usb_device_manager_uevent_cb), self);
> - /* Do coldplug (detection of already connected devices) */
> - libusb_get_device_list(priv->context, &priv->coldplug_list);
> - list = g_udev_client_query_by_subsystem(priv->udev, "usb");
> - for (it = g_list_first(list); it; it = g_list_next(it)) {
> - spice_usb_device_manager_add_udev(self, it->data);
> - g_object_unref(it->data);
> - }
> - g_list_free(list);
> - libusb_free_device_list(priv->coldplug_list, 1);
> - priv->coldplug_list = NULL;
> -#else
> rc = libusb_hotplug_register_callback(priv->context,
> LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED | LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT,
> LIBUSB_HOTPLUG_ENUMERATE, LIBUSB_HOTPLUG_MATCH_ANY,
> @@ -366,7 +340,7 @@ static void spice_usb_device_manager_dispose(GObject *gobject)
> SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(gobject);
> SpiceUsbDeviceManagerPrivate *priv = self->priv;
>
> -#ifndef USE_GUDEV
> +#ifndef G_OS_WIN32
> if (priv->hp_handle) {
> spice_usb_device_manager_stop_event_listening(self);
> if (g_atomic_int_get(&priv->event_thread_run)) {
> @@ -405,12 +379,15 @@ static void spice_usb_device_manager_finalize(GObject *gobject)
> g_ptr_array_unref(priv->devices);
>
> #ifdef USE_USBREDIR
> -#ifdef USE_GUDEV
> +#ifdef G_OS_WIN32
> g_clear_object(&priv->udev);
> #endif
> g_return_if_fail(priv->event_thread == NULL);
> +#ifndef G_OS_WIN32
> + /* under Windows we reuse existing libusb context */
> if (priv->context)
> libusb_exit(priv->context);
> +#endif
> free(priv->auto_conn_filter_rules);
> free(priv->redirect_on_connect_rules);
> #ifdef G_OS_WIN32
> @@ -735,29 +712,7 @@ static void spice_usb_device_manager_class_init(SpiceUsbDeviceManagerClass *klas
> #ifdef USE_USBREDIR
>
> /* ------------------------------------------------------------------ */
> -/* gudev / libusb Helper functions */
> -
> -#ifdef USE_GUDEV
> -static gboolean spice_usb_device_manager_get_udev_bus_n_address(
> - SpiceUsbDeviceManager *manager, GUdevDevice *udev,
> - int *bus, int *address)
> -{
> - const gchar *bus_str, *address_str;
> -
> - *bus = *address = 0;
> -
> - /* Linux or UsbDk backend on Windows*/
> - bus_str = g_udev_device_get_property(udev, "BUSNUM");
> - address_str = g_udev_device_get_property(udev, "DEVNUM");
> -
> - if (bus_str)
> - *bus = atoi(bus_str);
> - if (address_str)
> - *address = atoi(address_str);
> -
> - return *bus && *address;
> -}
> -#endif
> +/* libusb Helper functions */
>
> static gboolean spice_usb_device_manager_get_device_descriptor(
> libusb_device *libdev,
> @@ -927,17 +882,6 @@ spice_usb_device_manager_device_match(SpiceUsbDeviceManager *self, SpiceUsbDevic
> spice_usb_device_get_devaddr(device) == address);
> }
>
> -#ifdef USE_GUDEV
> -static gboolean
> -spice_usb_device_manager_libdev_match(SpiceUsbDeviceManager *self, libusb_device *libdev,
> - const int bus, const int address)
> -{
> - /* match functions for Linux/UsbDk -- match by bus.addr */
> - return (libusb_get_bus_number(libdev) == bus &&
> - libusb_get_device_address(libdev) == address);
> -}
> -#endif
> -
> static SpiceUsbDevice*
> spice_usb_device_manager_find_device(SpiceUsbDeviceManager *self,
> const int bus, const int address)
> @@ -970,6 +914,16 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> if (desc.bDeviceClass == LIBUSB_CLASS_HUB)
> return;
>
> + if (spice_usb_device_manager_find_device(self,
> + libusb_get_bus_number(libdev),
> + libusb_get_device_address(libdev))) {
> + SPICE_DEBUG("device not added %04x:%04x (%p)",
> + desc.idVendor,
> + desc.idProduct,
> + libdev);
> + return;
> + }
> +
I did not understand why we need this?
> device = (SpiceUsbDevice*)spice_usb_device_new(libdev);
> if (!device)
> return;
> @@ -994,10 +948,11 @@ static void spice_usb_device_manager_add_dev(SpiceUsbDeviceManager *self,
> spice_usb_device_ref(device));
> }
>
> - SPICE_DEBUG("device added %04x:%04x (%p)",
> + SPICE_DEBUG("device added %04x:%04x (%p, libusb %p)",
> spice_usb_device_get_vid(device),
> spice_usb_device_get_pid(device),
> - device);
> + device,
> + libdev);
> g_signal_emit(self, signals[DEVICE_ADDED], 0, device);
> }
>
> @@ -1027,80 +982,20 @@ static void spice_usb_device_manager_remove_dev(SpiceUsbDeviceManager *self,
> spice_usb_device_unref(device);
> }
>
> -#ifdef USE_GUDEV
> -static void spice_usb_device_manager_add_udev(SpiceUsbDeviceManager *self,
> - GUdevDevice *udev)
> -{
> - SpiceUsbDeviceManagerPrivate *priv = self->priv;
> - libusb_device *libdev = NULL, **dev_list = NULL;
> - SpiceUsbDevice *device;
> - const gchar *devtype;
> - int i, bus, address;
> -
> - devtype = g_udev_device_get_property(udev, "DEVTYPE");
> - /* Check if this is a usb device (and not an interface) */
> - if (!devtype || strcmp(devtype, "usb_device"))
> - return;
> -
> - if (!spice_usb_device_manager_get_udev_bus_n_address(self, udev, &bus, &address)) {
> - g_warning("USB device without bus number or device address");
> - return;
> - }
> -
> - device = spice_usb_device_manager_find_device(self, bus, address);
> - if (device) {
> - SPICE_DEBUG("USB device 0x%04x:0x%04x at %d.%d already exists, ignored",
> - spice_usb_device_get_vid(device),
> - spice_usb_device_get_pid(device),
> - spice_usb_device_get_busnum(device),
> - spice_usb_device_get_devaddr(device));
> - return;
> - }
> -
> - if (priv->coldplug_list)
> - dev_list = priv->coldplug_list;
> - else
> - libusb_get_device_list(priv->context, &dev_list);
> -
> - for (i = 0; dev_list && dev_list[i]; i++) {
> - if (spice_usb_device_manager_libdev_match(self, dev_list[i], bus, address)) {
> - libdev = dev_list[i];
> - break;
> - }
> - }
> -
> - if (libdev)
> - spice_usb_device_manager_add_dev(self, libdev);
> - else
> - g_warning("Could not find USB device to add " DEV_ID_FMT,
> - (guint) bus, (guint) address);
> -
> - if (!priv->coldplug_list)
> - libusb_free_device_list(dev_list, 1);
> -}
> -
> -static void spice_usb_device_manager_remove_udev(SpiceUsbDeviceManager *self,
> - GUdevDevice *udev)
> -{
> - int bus, address;
> -
> - if (!spice_usb_device_manager_get_udev_bus_n_address(self, udev, &bus, &address))
> - return;
> -
> - spice_usb_device_manager_remove_dev(self, bus, address);
> -}
> -
> +#ifdef G_OS_WIN32
> static void spice_usb_device_manager_uevent_cb(GUdevClient *client,
> - const gchar *action,
> - GUdevDevice *udevice,
> + libusb_device *libdev,
> + gboolean add,
> gpointer user_data)
> {
> SpiceUsbDeviceManager *self = SPICE_USB_DEVICE_MANAGER(user_data);
>
> - if (g_str_equal(action, "add"))
> - spice_usb_device_manager_add_udev(self, udevice);
> - else if (g_str_equal (action, "remove"))
> - spice_usb_device_manager_remove_udev(self, udevice);
> + if (add)
> + spice_usb_device_manager_add_dev(self, libdev);
> + else
> + spice_usb_device_manager_remove_dev(self,
> + libusb_get_bus_number(libdev),
> + libusb_get_device_address(libdev));
This could almost reuse spice_usb_device_manager_hotplug_cb somehow, but
I'm not sure this is worth it.
> }
> #else
> struct hotplug_idle_cb_args {
> @@ -1240,10 +1135,6 @@ static void spice_usb_device_manager_check_redir_on_connect(
> continue;
>
> libdev = spice_usb_device_manager_device_to_libdev(self, device);
> -#ifdef G_OS_WIN32
> - if (libdev == NULL)
> - continue;
> -#endif
> if (usbredirhost_check_device_filter(
> priv->redirect_on_connect_rules,
> priv->redirect_on_connect_rules_count,
> @@ -1348,10 +1239,6 @@ GPtrArray* spice_usb_device_manager_get_devices_with_filter(
> if (rules) {
> libusb_device *libdev =
> spice_usb_device_manager_device_to_libdev(self, device);
> -#ifdef G_OS_WIN32
> - if (libdev == NULL)
> - continue;
> -#endif
> if (usbredirhost_check_device_filter(rules, count, libdev, 0) != 0)
> continue;
> }
> @@ -1443,24 +1330,6 @@ _spice_usb_device_manager_connect_device_async(SpiceUsbDeviceManager *self,
> continue; /* Skip already used channels */
>
> libdev = spice_usb_device_manager_device_to_libdev(self, device);
> -#ifdef G_OS_WIN32
> - if (libdev == NULL) {
> - /* Most likely, the device was plugged out at driver installation
> - * time, and its remove-device event was ignored.
> - * So remove the device now
> - */
> - SPICE_DEBUG("libdev does not exist for %p -- removing", device);
> - spice_usb_device_ref(device);
> - g_ptr_array_remove(priv->devices, device);
> - g_signal_emit(self, signals[DEVICE_REMOVED], 0, device);
> - spice_usb_device_unref(device);
> - g_task_return_new_error(task,
> - SPICE_CLIENT_ERROR,
> - SPICE_CLIENT_ERROR_FAILED,
> - _("Device was not found"));
> - goto done;
> - }
> -#endif
> spice_usbredir_channel_connect_device_async(channel,
> libdev,
> device,
> @@ -1732,13 +1601,6 @@ spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager *self,
> libusb_device *libdev;
>
> libdev = spice_usb_device_manager_device_to_libdev(self, device);
> -#ifdef G_OS_WIN32
> - if (libdev == NULL) {
> - g_set_error_literal(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> - _("Some USB devices were not found"));
> - return FALSE;
> - }
> -#endif
> filter_ok = (usbredirhost_check_device_filter(
> guest_filter_rules, guest_filter_rules_count,
> libdev, 0) == 0);
> @@ -1889,9 +1751,7 @@ static SpiceUsbDeviceInfo *spice_usb_device_new(libusb_device *libdev)
> info->pid = pid;
> info->ref = 1;
> info->isochronous = probe_isochronous_endpoint(libdev);
> -#ifndef G_OS_WIN32
> info->libdev = libusb_ref_device(libdev);
> -#endif
>
> return info;
> }
> @@ -2027,14 +1887,11 @@ static void spice_usb_device_unref(SpiceUsbDevice *device)
>
> ref_count_is_0 = g_atomic_int_dec_and_test(&info->ref);
> if (ref_count_is_0) {
> -#ifndef G_OS_WIN32
> libusb_unref_device(info->libdev);
> -#endif
Most of the 'persistent' libusb_device changes for Windows are in the
hunks before this, but they also depend on some rework of the hotplug
detection in GUdevClient.
> g_free(info);
> }
> }
>
> -#ifndef G_OS_WIN32 /* Linux -- directly compare libdev */
> static gboolean
> spice_usb_manager_device_equal_libdev(SpiceUsbDeviceManager *manager,
> SpiceUsbDevice *device,
> @@ -2047,23 +1904,6 @@ spice_usb_manager_device_equal_libdev(SpiceUsbDeviceManager *manager,
>
> return info->libdev == libdev;
> }
> -#else /* Windows -- compare vid:pid of device and libdev */
> -static gboolean
> -spice_usb_manager_device_equal_libdev(SpiceUsbDeviceManager *manager,
> - SpiceUsbDevice *device,
> - libusb_device *libdev)
> -{
> - int busnum, devaddr;
> -
> - if ((device == NULL) || (libdev == NULL))
> - return FALSE;
> -
> - busnum = spice_usb_device_get_busnum(device);
> - devaddr = spice_usb_device_get_devaddr(device);
> - return spice_usb_device_manager_libdev_match(manager, libdev,
> - busnum, devaddr);
> -}
> -#endif
>
> /*
> * Caller must libusb_unref_device the libusb_device returned by this function.
> @@ -2073,42 +1913,8 @@ static libusb_device *
> spice_usb_device_manager_device_to_libdev(SpiceUsbDeviceManager *self,
> SpiceUsbDevice *device)
> {
> -#ifdef G_OS_WIN32
> - /*
> - * On win32 we need to do this the hard and slow way, by asking libusb to
> - * re-enumerate all devices and then finding a matching device.
> - * We cannot cache the libusb_device like we do under Linux since the
> - * driver swap we do under windows invalidates the cached libdev.
> - */
> -
> - libusb_device *d, **devlist;
> - int i;
> -
> - g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self), NULL);
> - g_return_val_if_fail(device != NULL, NULL);
> - g_return_val_if_fail(self->priv != NULL, NULL);
> - g_return_val_if_fail(self->priv->context != NULL, NULL);
> -
> - libusb_get_device_list(self->priv->context, &devlist);
> - if (!devlist)
> - return NULL;
> -
> - for (i = 0; (d = devlist[i]) != NULL; i++) {
> - if (spice_usb_manager_device_equal_libdev(self, device, d)) {
> - libusb_ref_device(d);
> - break;
> - }
> - }
> -
> - libusb_free_device_list(devlist, 1);
> -
> - return d;
> -
> -#else
> /* Simply return a ref to the cached libdev */
> SpiceUsbDeviceInfo *info = (SpiceUsbDeviceInfo *)device;
> -
> return libusb_ref_device(info->libdev);
> -#endif
> }
> #endif /* USE_USBREDIR */
> diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> index 327976d..ef6804f 100644
> --- a/src/win-usb-dev.c
> +++ b/src/win-usb-dev.c
> @@ -49,31 +49,6 @@ G_DEFINE_TYPE_WITH_CODE(GUdevClient, g_udev_client, G_TYPE_OBJECT,
> G_ADD_PRIVATE(GUdevClient)
> G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE, g_udev_client_initable_iface_init))
>
> -
> -typedef struct _GUdevDeviceInfo GUdevDeviceInfo;
> -
> -struct _GUdevDeviceInfo {
> - guint16 bus;
> - guint16 addr;
> - guint16 vid;
> - guint16 pid;
> - guint16 class;
> - gchar sclass[4];
> - gchar sbus[4];
> - gchar saddr[4];
> - gchar svid[8];
> - gchar spid[8];
> -};
> -
> -struct _GUdevDevicePrivate
> -{
> - /* FixMe: move above fields to this structure and access them directly */
> - GUdevDeviceInfo *udevinfo;
> -};
> -
> -G_DEFINE_TYPE_WITH_PRIVATE(GUdevDevice, g_udev_device, G_TYPE_OBJECT)
For what it's worth, adding a libusb_device pointer to
GUdevDevicePrivate and using this to implement the 'persistent device'
thing on Windows, and removing GUdevDevice/GUdevInfo in followup commits
greatly reduce the overall size of the patch.
> -
> -
> enum
> {
> UEVENT_SIGNAL,
> @@ -83,33 +58,35 @@ enum
> static guint signals[LAST_SIGNAL] = { 0, };
> static GUdevClient *singleton = NULL;
>
> -static GUdevDevice *g_udev_device_new(GUdevDeviceInfo *udevinfo);
> static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam);
> -static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *udevinfo);
>
> //uncomment to debug gudev device lists.
> -//#define DEBUG_GUDEV_DEVICE_LISTS
> +#define DEBUG_GUDEV_DEVICE_LISTS
This change is unneeded in production.
>
> #ifdef DEBUG_GUDEV_DEVICE_LISTS
> static void g_udev_device_print_list(GList *l, const gchar *msg);
> #else
> static void g_udev_device_print_list(GList *l, const gchar *msg) {}
> #endif
> -static void g_udev_device_print(GUdevDevice *udev, const gchar *msg);
> +static void g_udev_device_print(libusb_device *dev, const gchar *msg);
>
> -static gboolean g_udev_skip_search(GUdevDevice *udev);
> +static gboolean g_udev_skip_search(libusb_device *dev);
>
> GQuark g_udev_client_error_quark(void)
> {
> return g_quark_from_static_string("win-gudev-client-error-quark");
> }
>
> -GUdevClient *g_udev_client_new(const gchar* const *subsystems)
> +GUdevClient *g_udev_client_new(libusb_context **pctx)
> {
> - if (singleton != NULL)
> + if (singleton != NULL) {
> + *pctx = singleton->priv->ctx;
> return g_object_ref(singleton);
Having a g_udev_client_new(void) +
g_udev_client_get_libusb_context(UdevClient *)
is more common in gobject APIs.
> -
> + }
> singleton = g_initable_new(G_UDEV_TYPE_CLIENT, NULL, NULL, NULL);
> + if (singleton) {
> + *pctx = singleton->priv->ctx;
> + }
> return singleton;
> }
>
> @@ -125,8 +102,6 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
> 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);
> @@ -148,14 +123,10 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
>
> n = 0;
> for (dev = lusb_list; *dev; dev++) {
> - udevinfo = g_new0(GUdevDeviceInfo, 1);
> - get_usb_dev_info(*dev, udevinfo);
> - udevice = g_udev_device_new(udevinfo);
> - if (g_udev_skip_search(udevice)) {
> - g_object_unref(udevice);
> + if (g_udev_skip_search(*dev)) {
> continue;
> }
> - *devs = g_list_prepend(*devs, udevice);
> + *devs = g_list_prepend(*devs, libusb_ref_device(*dev));
> n++;
> }
> libusb_free_device_list(lusb_list, 1);
> @@ -163,11 +134,17 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
> return n;
> }
>
> +static void unreference_libusb_device(gpointer data)
> +{
> + libusb_unref_device((libusb_device *)data);
> +}
> +
> 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);
> + /* avoiding casting of imported procedure to GDestroyNotify */
> + g_list_free_full(*devs, unreference_libusb_device);
> *devs = NULL;
> }
> }
> @@ -243,11 +220,16 @@ static void g_udev_client_initable_iface_init(GInitableIface *iface)
> iface->init = g_udev_client_initable_init;
> }
>
> -GList *g_udev_client_query_by_subsystem(GUdevClient *self, const gchar *subsystem)
> +static void report_one_device(gpointer data, gpointer user_data)
> +{
> + GUdevClient *self = user_data;
> + g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);
> +}
> +
> +void g_udev_client_report_devices(GUdevClient *self)
> {
> GList *l = g_list_copy(self->priv->udev_list);
> - g_list_foreach(l, (GFunc)g_object_ref, NULL);
> - return l;
> + g_list_foreach(l, report_one_device, self);
g_list_free(l);
> }
>
> static void g_udev_client_init(GUdevClient *self)
> @@ -335,11 +317,11 @@ static void g_udev_client_class_init(GUdevClientClass *klass)
> G_SIGNAL_RUN_FIRST,
> G_STRUCT_OFFSET(GUdevClientClass, uevent),
> NULL, NULL,
> - g_cclosure_user_marshal_VOID__BOXED_BOXED,
> + g_cclosure_user_marshal_VOID__POINTER_INT,
libusb_device could/should be boxed
> G_TYPE_NONE,
> 2,
> - G_TYPE_STRING,
> - G_UDEV_TYPE_DEVICE);
> + G_TYPE_POINTER,
> + G_TYPE_INT);
>
> /**
> * GUdevClient::redirecting:
> @@ -356,41 +338,22 @@ static void g_udev_client_class_init(GUdevClientClass *klass)
> g_object_class_install_property(gobject_class, PROP_REDIRECTING, pspec);
> }
>
> -static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *udevinfo)
> +/* Only bus:addr are compared */
> +static gint compare_libusb_devices(gconstpointer a, gconstpointer b)
> {
> - struct libusb_device_descriptor desc;
> -
> - g_return_val_if_fail(dev, FALSE);
> - g_return_val_if_fail(udevinfo, FALSE);
> + struct {
> + uint8_t bus;
> + uint8_t addr;
> + } a_info, b_info, *ai = &a_info, *bi = &b_info;
No need for this intermediate structure or pointers:
a_bus = libusb_get_bus_number(dev);
b_bus = libusb_get_bus_number(dev);
a_addr = ...
b_addr = ...
>
> - 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;
> - g_snprintf(udevinfo->sclass, sizeof(udevinfo->sclass), "%d", udevinfo->class);
> - g_snprintf(udevinfo->sbus, sizeof(udevinfo->sbus), "%d", udevinfo->bus);
> - g_snprintf(udevinfo->saddr, sizeof(udevinfo->saddr), "%d", udevinfo->addr);
> - g_snprintf(udevinfo->svid, sizeof(udevinfo->svid), "%d", udevinfo->vid);
> - g_snprintf(udevinfo->spid, sizeof(udevinfo->spid), "%d", udevinfo->pid);
> - return TRUE;
> -}
> + ai->bus = libusb_get_bus_number((libusb_device *)a);
> + ai->addr = libusb_get_device_address((libusb_device *)a);
> + bi->bus = libusb_get_bus_number((libusb_device *)b);
> + bi->addr = libusb_get_device_address((libusb_device *)b);
>
> -/* Only bus:addr are compared */
> -static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
> -{
> - GUdevDeviceInfo *ai, *bi;
> gboolean same_bus;
> gboolean same_addr;
>
> - ai = G_UDEV_DEVICE(a)->priv->udevinfo;
> - bi = G_UDEV_DEVICE(b)->priv->udevinfo;
> -
> same_bus = (ai->bus == bi->bus);
> same_addr = (ai->addr == bi->addr);
>
> @@ -400,15 +363,21 @@ static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
> static void notify_dev_state_change(GUdevClient *self,
> GList *old_list,
> GList *new_list,
> - const gchar *action)
> + gboolean add)
> {
> GList *dev;
>
> for (dev = g_list_first(old_list); dev != NULL; dev = g_list_next(dev)) {
> - if (g_list_find_custom(new_list, dev->data, gudev_devices_differ) == NULL) {
> + GList *found = g_list_find_custom(new_list, dev->data, compare_libusb_devices);
> + if (found == NULL) {
> /* Found a device that changed its state */
> - g_udev_device_print(dev->data, action);
> - g_signal_emit(self, signals[UEVENT_SIGNAL], 0, action, dev->data);
> + g_udev_device_print(dev->data, add ? "add" : "remove");
> + g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, add);
> + } else if (add) {
> + /* keep existing device in new list */
> + GList *temp = found->data;
This is a libusb_device *, not a GList *
> + found->data = dev->data;
> + dev->data = temp;
Looking at handle_dev_change for some context, this code is triggered
there:
notify_dev_state_change(self, now_devs, priv->udev_list, TRUE);
/* keep most recent info: free previous list, and keep current list */
g_udev_client_free_device_list(&priv->udev_list);
priv->udev_list = now_devs;
'found->data' would be coming from 'priv->udev_list' and 'dev->data'
from 'now_devs'
After this code runs, we'll be keeping the 'now_devs' list so when we
get a device change notification, if we have a device which we already
knew of (same bus/addr), but represented by a potentially different
libusb_device *, we'll be keeping the old libusb_device *, not the newer
one.
This has me worried as we are removing a comment saying:
* On win32 we need to do this the hard and slow way, by asking libusb to
* re-enumerate all devices and then finding a matching device.
* We cannot cache the libusb_device like we do under Linux since the
* driver swap we do under windows invalidates the cached libdev.
*/
and I initially thought this code was precisely fixing that issue.
Do you remember why you added this 'else if (add)'?
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel