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

Yuri Benditovich yuri.benditovich at daynix.com
Mon Jan 21 14:10:20 UTC 2019


On Fri, Jan 18, 2019 at 3:53 PM Christophe Fergeau <cfergeau at redhat.com>
wrote:

> 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


This commit was discarder by further changes and related to some problem
with WinUSB and there is
no other information about it. If the device is moved from one bus to
another one, this is regular PnP event
and it should be processed as such.



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

Even if I plan to do so, the removal of reenumeration is not something that
passed the same agressive testing as exising patch and I do not plan to bind
it with current patch. As current patch do not present any regression or
degradation, I suppose it does not request any changes in the
infrastructure.
Of course, it is possible that existing API does not satisfy you. But it
does not
look like this is the case.
Your suggestion is to move into 'backend' code part of the code from
usb-device-manager
that anyway should be removed. This would create significant changes in the
current patch
and require additional round of the tests. This is why I do not agree.


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

I do not know whuch 'vague promises' you mention.
Possible, this is not related to the matter.


> 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.
>
>
I've already sent you the illustration how this can be done.
See last 3 commits at
https://gitlab.freedesktop.org/yuri_benditovich/spice-gtk/commits/pers-dev
But this is NOT what I'm submitting right now.


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


More information about the Spice-devel mailing list