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

[Christophe Fergeau]

On Fri, Jan 18, 2019 at 10:23:04AM +0200, Yuri Benditovich wrote:
> 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.

The libusb object does not disappear, but apparently there were some
situations where the device 'moved', and the libusb object pointed to a
no-longer existing device, thus the need to enumerate again.
I believe this is related to
https://gitlab.freedesktop.org/spice/spice-gtk/commit/76e94509cf29a78aa39740c81dcdd2eee355c7b9
https://bugzilla.redhat.com/show_bug.cgi?id=842816


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

I'm not suggesting to squash everything in the current patch, but we are
not limited to one single patch. If you intend to remove that
persistency code right after the patch being discussed, it would help to
send the 2 patches as a series.
Regarding the unification of the structure holding device properties,
since you are introducing the duplication in that patch, I'd rather that
we don't add suboptimal code with vague promises that it's going to be
fixed later.
If you point me at a branch with all your patches applied including this
one, I can try to help with some of the preparatory commits/some of the
splitting.

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/20190118/99b0fad4/attachment.sig>