On Wed, Feb 20, 2019 at 6:03 PM Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
>
> 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.
>
No, this window does not have its own thread and its messages are
dispatched/retrieved by main window procedure. And anyway, if due to some
misunderstanding (see below) the same device is signaled twice, it
will be filtered
out by usb-device-manager.
> 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?
There are several reasons:
1. It is possible that the hot plug procedure for Linux might be
called more than once for the same device (this is mentioned in libusb
API).
2. If second instance of usb-device-manager created (as it happens at
time of remote-viewer termination),
existing one receives each device that already present.
3. It is much simpler to exclude device duplication than later look
why they present.
>
>
> > 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.
What I'm expected to do to address this note?
>
> > }
> > #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.
What I'm expected to do to address this note?
>
> > 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.
>
If you find this mandatory, please reject this patch and clearly
request to provide
patches that are not bigger than N lines.
> > -
> > -
> > 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.
This is my mistake, it will be fixed in v2 if it happens.
>
> >
> > #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.
>
I do not see any reason to create 2 APIs where one is sufficient and
easy for error handling.
If this change is mandatory, please state it clearly.
> > -
> > + }
> > 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
I do not see any reason for additional ref/unref for libusb_device.
If you can provide one, please do.
If you find this change mandatory for the patch to be accepted, please
state it clearly.
>
> > 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 = ...
>
This is done to make the patch more readable.
If changing this is mandatory, please let me know.
>
> >
> > - 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 *
OK, I will fix it in v2, if it happens.
>
> > + 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)'?
The goal is to keep this device list and device list in usb-device-manager fully
synchronized. If due to some misunderstanding there is the case with the same
bus:addr but different devices, also previous code will not detect it
and will not signal
device change to the usb-device-manager. This can be improved by checking
also vendor:product. But the goal of this patch is not to improve
existing code, but
use persistent libusb_device.
As far as I understand things - nothing can _invalidate_ referenced libdev.
>
> Christophe
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel