On Sun, Mar 10, 2019 at 04:46:09PM +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.benditov...@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 1ab704d..5d878ea 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 */
> +        g_list_free_full(*devs, unreference_libusb_device);

Since all unreference_libusb_device is doing is blindly casting a void *
to libusb_device *, I'd just use a cast to GDestroyNotify here rather
than introduce an intermediate method. If you prefer to keep that
intermediate helper, please remove the commit which I don't think is
needed.

>          *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, int 
> 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;

Is this chunk required in this commit for proper behaviour the usb code?
Or will it be required in some follow-up commit? If it's only going to
be required later, I'd squash this bit in the commit where it's needed
so that it's clearer why we want this.

Apart from these,

Acked-by: Christophe Fergeau <cferg...@redhat.com>

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Reply via email to