[Spice-devel] [spice-gtk v2 10/13] win-usb-dev: maintain list of libusb devices

[Yuri Benditovich]

On Thu, Mar 21, 2019 at 3:19 PM Christophe Fergeau <cfergeau at redhat.com> wrote:
>
> On Tue, Mar 19, 2019 at 07:56:05AM +0200, Yuri Benditovich wrote:
> > Change internal device list to maintain libusb devices
> > instead of GUdevDevice objects. Create temporary
> > GUdevDevice object only for indication to usb-dev-manager.
> >
> > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > ---
> >  src/win-usb-dev.c | 80 ++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 51 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > index b3b2ed8..0b87e75 100644
> > --- a/src/win-usb-dev.c
> > +++ b/src/win-usb-dev.c
> > @@ -95,7 +95,7 @@ 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(libusb_device *dev);
> >
> > @@ -129,8 +129,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);
> > @@ -155,10 +153,7 @@ g_udev_client_list_devices(GUdevClient *self, GList **devs,
> >          if (g_udev_skip_search(*dev)) {
> >              continue;
> >          }
> > -        udevinfo = g_new0(GUdevDeviceInfo, 1);
> > -        get_usb_dev_info(*dev, udevinfo);
> > -        udevice = g_udev_device_new(udevinfo);
> > -        *devs = g_list_prepend(*devs, udevice);
> > +        *devs = g_list_prepend(*devs, libusb_ref_device(*dev));
> >          n++;
> >      }
> >      libusb_free_device_list(lusb_list, 1);
> > @@ -166,11 +161,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 */
>
> Maybe make this very explicit with something like this?
>
>   /* the unreference_libusb_device method is required as
>    * libusb_unref_device calling convention differs from glib's
>    * see 558c967ec
>    */
>

>From my point of view this is obvious and existing comment is enough.
Anyway, please feel free to change the comment as you want.

>
>
> > +        g_list_free_full(*devs, unreference_libusb_device);
> >          *devs = NULL;
> >      }
> >  }
> > @@ -246,9 +247,22 @@ static void g_udev_client_initable_iface_init(GInitableIface *iface)
> >      iface->init = g_udev_client_initable_init;
> >  }
> >
> > +static void g_udev_notify_device(GUdevClient *self, libusb_device *dev, gboolean add)
> > +{
> > +    GUdevDeviceInfo *udevinfo;
> > +    GUdevDevice *udevice;
> > +    udevinfo = g_new0(GUdevDeviceInfo, 1);
> > +    if (get_usb_dev_info(dev, udevinfo)) {
> > +        udevice = g_udev_device_new(udevinfo);
> > +        g_signal_emit(self, signals[UEVENT_SIGNAL], 0, udevice, add);
> > +    } else {
> > +        g_free(udevinfo);
> > +    }
> > +}
> > +
> >  static void report_one_device(gpointer data, gpointer self)
> >  {
> > -    g_signal_emit(self, signals[UEVENT_SIGNAL], 0, data, TRUE);
> > +    g_udev_notify_device(self, data, TRUE);
> >  }
> >
> >  void g_udev_client_report_devices(GUdevClient *self)
> > @@ -388,18 +402,20 @@ static gboolean get_usb_dev_info(libusb_device *dev, GUdevDeviceInfo *udevinfo)
> >  }
> >
> >  /* comparing bus:addr and vid:pid */
> > -static gint gudev_devices_differ(gconstpointer a, gconstpointer b)
> > +static gint compare_libusb_devices(gconstpointer a, gconstpointer b)
> >  {
> > -    GUdevDeviceInfo *ai, *bi;
> > +    libusb_device *a_dev = (libusb_device *)a;
> > +    libusb_device *b_dev = (libusb_device *)b;
> > +    struct libusb_device_descriptor a_desc, b_desc;
> >      gboolean same_bus, same_addr, same_vid, same_pid;
> >
> > -    ai = G_UDEV_DEVICE(a)->priv->udevinfo;
> > -    bi = G_UDEV_DEVICE(b)->priv->udevinfo;
> > +    libusb_get_device_descriptor(a_dev, &a_desc);
> > +    libusb_get_device_descriptor(b_dev, &b_desc);
> >
> > -    same_bus = (ai->bus == bi->bus);
> > -    same_addr = (ai->addr == bi->addr);
> > -    same_vid = (ai->vid == bi->vid);
> > -    same_pid = (ai->pid == bi->pid);
> > +    same_bus = (libusb_get_bus_number(a_dev) == libusb_get_bus_number(b_dev));
> > +    same_addr = (libusb_get_device_address(a_dev) == libusb_get_device_address(b_dev));
> > +    same_vid = (a_desc.idVendor == b_desc.idVendor);
> > +    same_pid = (a_desc.idProduct == b_desc.idProduct);
> >
> >      return (same_bus && same_addr && same_vid && same_pid) ? 0 : -1;
> >  }
> > @@ -412,10 +428,17 @@ static void notify_dev_state_change(GUdevClient *self,
> >      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) {
> > -            /* Found a device that changed its state */
> > +        GList *found = g_list_find_custom(new_list, dev->data, compare_libusb_devices);
> > +        if (found == NULL) {
> >              g_udev_device_print(dev->data, add ? "add" : "remove");
> > -            g_signal_emit(self, signals[UEVENT_SIGNAL], 0, dev->data, add);
> > +            g_udev_notify_device(self, dev->data, add);
> > +        } else if (add) {
> > +            /* keep old existing libusb_device in the new list, when
> > +               usb-dev-manager will maintain its own list of libusb_device,
> > +               these lists will be completely consistent */
> > +            libusb_device *temp = found->data;
> > +            found->data = dev->data;
> > +            dev->data = temp;
>
> I'm still annoyed by this slightly complicated code in a method named notify_dev_state_change
> (more on this below)
>
> >          }
> >      }
> >  }
> > @@ -446,7 +469,8 @@ static void handle_dev_change(GUdevClient *self)
> >      /* Unregister devices that are not present anymore */
> >      notify_dev_state_change(self, priv->udev_list, now_devs, FALSE);
> >
> > -    /* Register newly inserted devices */
> > +    /* report newly inserted devices and swap pointers to existing devices:
> > +       keep old pointers in now_devs list, keep new pointers in udev_list */
> >      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;
>
> Maybe these 2 lines + the code to replace new libusb_device pointers
> with the old ones could be moved to a new
> g_udev_client_update_device_list(self, now_devs);
> helper?
>

>From my point of view, this will make the code more complicated,
as additional procedure should traverse both lists again.

> Christophe