On Fri, Jan 18, 2019 at 3:53 PM Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > > 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
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel