[Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer

[Christophe Fergeau]

On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau <cfergeau at redhat.com>
> wrote:
> > In _SpiceUsbDeviceManagerPrivate, you replaced
> > #ifndef G_OS_WIN32
> >     libusb_device *libdev;
> > #endif
> >
> > with
> >
> > #ifndef G_OS_WIN32
> >     SpiceUsbBackendDevice *bdev;
> > #endif
> >
> > The #ifdef is there because of this comment in
> > spice_usb_device_manager_device_to_bdev:
> >
> >   /*
> >    * On win32 we need to do this the hard and slow way, by asking libusb to
> >    * re-enumerate all devices and then finding a matching device.
> >    * We cannot cache the libusb_device like we do under Linux since the
> >    * driver swap we do under windows invalidates the cached libdev.
> >    */
> >
> > After your patch, spice_usb_device_manager_device_to_bdev is no longer
> > at the right level of indirection imo, it looks up the 'bdev' when
> > needed on Windows in usb-device-manager.c, but my understanding of that
> > comment is that any libusb call within SpiceUsbBackendDevice should not
> > use a cached libusb_device?
> >
> >
> >
> As fas as I understand, there is no change in the level of indirection.
> Previously was:
> On Linux: usb-device-manager receives libusb device on hotplug indication
> and uses it until the device disappears.
> On Windows: each time the usb-device-manager needs the libusb device it
> takes fresh list of libusb devices and finds one according to known device
> properties.
> 
> Now:
> On Linux: usb-device-manager receives backend device wrapping libusb device
> on hotplug indication and uses it until the device disappears
> On Windows: each time the usb-device-manager needs the backend device it
> takes fresh list of new backend devices and finds one according to known
> device properties.
> So, the backend device is always fresh one and it always have fresh libusb
> device.
> 
> This can be simplified with removal of all these lookups in the
> usb-device-manager (and I already illustrated how) but after this commit is
> done.
> If from your point of view this is the requirement to existing commit,
> please let me know (then we will need to reevaluate our plans and our
> ability to deliver the patches in reasonable time).

I'll quote again the comment:

   /*
    * On win32 we need to do this the hard and slow way, by asking libusb to
    * re-enumerate all devices and then finding a matching device.
    * We cannot cache the libusb_device like we do under Linux since the
    * driver swap we do under windows invalidates the cached libdev.
    */

It says we cannot cache a libusb_device because it can change behind our
back, so we need to reenumerate all devices and lookup the right
libusb_device before every call to libusb.
Before your changes, we did not hold any long-lasting references to a
libusb_device on Windows, so we were guaranteed to do that lookup before
every call. After your changes, SpiceUsbBackendDevice is holding a
long-lasting reference to a libusb_device, but does not care about the
win32 issue. Only usb-device-manager is making sure the libusb_device
it's going to use is valid. This does not make sense to me to leak this
implementation detail to usb-device-manager. SpiceUsbBackend should hide
it, otherwise its API is going to be error-prone.

When you say this can be simplified by removing this lookup, are you
confirming that this comment I quoted above is obsolete?

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/20190116/2e7e8623/attachment.sig>