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

[Christophe Fergeau]

On Wed, Mar 13, 2019 at 09:36:06AM +0200, Yuri Benditovich wrote:
> On Tue, Mar 12, 2019 at 7:22 PM Christophe Fergeau <cfergeau at redhat.com> 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 <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 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190313/18f821f2/attachment-0001.sig>