Re: [spice-gtk] usb-redir: use persistent libusb device on Windows

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]