Re: [Spice-devel] [spice-gtk 10/13] win-usb-dev: maintain list of libusb devices
On Tue, Mar 12, 2019 at 7:22 PM Christophe Fergeau
wrote:>> 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
> > ---
> > 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, _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 &&
Re: [Spice-devel] [spice-gtk 10/13] win-usb-dev: maintain list of libusb devices
On Wed, Mar 13, 2019 at 09:36:06AM +0200, Yuri Benditovich wrote:
> On Tue, Mar 12, 2019 at 7:22 PM Christophe Fergeau
> wrote:
> >
> > 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
> > > ---
> > > 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
>
> unreference_libusb_device not only 'blindly casts' pointers but also
> guarantees that libusb_unref_device is called with proper calling
> conventions. There was commit addressing similar issue
> https://gitlab.freedesktop.org/spice/spice-gtk/commit/558c967ecd230fa6ddde553f6206b1bfd86b40e7
oh, very good point, this is in my opinion important enough to be
mentioned in the commit log, and maybe in the comment.
>
> > intermediate helper, please remove the commit which I don't think is
> > needed.
>
> Which exact commit is not needed??
Sorry, I meant 'comment', not 'commit' :)
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 10/13] win-usb-dev: maintain list of libusb devices
On Tue, Mar 12, 2019 at 7:22 PM Christophe Fergeau wrote:
>
> 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
> > ---
> > 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
unreference_libusb_device not only 'blindly casts' pointers but also
guarantees that libusb_unref_device is called with proper calling
conventions. There was commit addressing similar issue
https://gitlab.freedesktop.org/spice/spice-gtk/commit/558c967ecd230fa6ddde553f6206b1bfd86b40e7
> intermediate helper, please remove the commit which I don't think is
> needed.
Which exact commit is not 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, _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);
> > +
Re: [Spice-devel] [spice-gtk 10/13] win-usb-dev: maintain list of libusb devices
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
> ---
> 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, _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,
