Re: [Spice-devel] [spice-gtk v2 10/13] win-usb-dev: maintain list of libusb devices
On Thu, Mar 21, 2019 at 05:16:03PM +0200, Yuri Benditovich wrote: > > > @@ -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(>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. I made an attempt at that, see the 2 patches in answer to this thread. I did not test them though :-/ Let me know what you think. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v2 10/13] win-usb-dev: maintain list of libusb devices
On Thu, Mar 21, 2019 at 3:19 PM Christophe Fergeau 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 > > --- > > 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, _desc); > > +libusb_get_device_descriptor(b_dev, _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 =
Re: [Spice-devel] [spice-gtk v2 10/13] win-usb-dev: maintain list of libusb devices
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 > --- > 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 */ > +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, _desc); > +libusb_get_device_descriptor(b_dev, _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
[Spice-devel] [spice-gtk v2 10/13] win-usb-dev: maintain list of libusb devices
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 --- 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 */ +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, _desc); +libusb_get_device_descriptor(b_dev, _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