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

[Yuri Benditovich]

On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau <cfergeau at redhat.com>
wrote:

> 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?
>

This comment, IMO, is wrong and permanent enumeration is not mandatory.
None of existing objects can disappear if it is is referenced.
But the object must be used with proper libusb context.

Changes in existing patch are intentionally minimal to allow comparison
between
previous and existing code. After the current patch is merged I will be
able to solve
the problem of persistency as I should be, I believe (together with
unification of the
structure holding device properties). But mixing all these things together
discards
the current patch and I can't predict the time frame of next round.


> Christophe
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190118/a4f2009f/attachment.html>