<div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jan 18, 2019 at 3:53 PM Christophe Fergeau <<a href="mailto:cfergeau@redhat.com">cfergeau@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Jan 18, 2019 at 10:23:04AM +0200, Yuri Benditovich wrote:<br>
> On Wed, Jan 16, 2019 at 6:11 PM Christophe Fergeau <<a href="mailto:cfergeau@redhat.com" target="_blank">cfergeau@redhat.com</a>><br>
> wrote:<br>
> <br>
> > On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:<br>
> > > On Mon, Nov 26, 2018 at 7:41 PM Christophe Fergeau <<a href="mailto:cfergeau@redhat.com" target="_blank">cfergeau@redhat.com</a>><br>
> > > wrote:<br>
> > > > In _SpiceUsbDeviceManagerPrivate, you replaced<br>
> > > > #ifndef G_OS_WIN32<br>
> > > >     libusb_device *libdev;<br>
> > > > #endif<br>
> > > ><br>
> > > > with<br>
> > > ><br>
> > > > #ifndef G_OS_WIN32<br>
> > > >     SpiceUsbBackendDevice *bdev;<br>
> > > > #endif<br>
> > > ><br>
> > > > The #ifdef is there because of this comment in<br>
> > > > spice_usb_device_manager_device_to_bdev:<br>
> > > ><br>
> > > >   /*<br>
> > > >    * On win32 we need to do this the hard and slow way, by asking<br>
> > libusb to<br>
> > > >    * re-enumerate all devices and then finding a matching device.<br>
> > > >    * We cannot cache the libusb_device like we do under Linux since the<br>
> > > >    * driver swap we do under windows invalidates the cached libdev.<br>
> > > >    */<br>
> > > ><br>
> > > > After your patch, spice_usb_device_manager_device_to_bdev is no longer<br>
> > > > at the right level of indirection imo, it looks up the 'bdev' when<br>
> > > > needed on Windows in usb-device-manager.c, but my understanding of that<br>
> > > > comment is that any libusb call within SpiceUsbBackendDevice should not<br>
> > > > use a cached libusb_device?<br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > As fas as I understand, there is no change in the level of indirection.<br>
> > > Previously was:<br>
> > > On Linux: usb-device-manager receives libusb device on hotplug indication<br>
> > > and uses it until the device disappears.<br>
> > > On Windows: each time the usb-device-manager needs the libusb device it<br>
> > > takes fresh list of libusb devices and finds one according to known<br>
> > device<br>
> > > properties.<br>
> > ><br>
> > > Now:<br>
> > > On Linux: usb-device-manager receives backend device wrapping libusb<br>
> > device<br>
> > > on hotplug indication and uses it until the device disappears<br>
> > > On Windows: each time the usb-device-manager needs the backend device it<br>
> > > takes fresh list of new backend devices and finds one according to known<br>
> > > device properties.<br>
> > > So, the backend device is always fresh one and it always have fresh<br>
> > libusb<br>
> > > device.<br>
> > ><br>
> > > This can be simplified with removal of all these lookups in the<br>
> > > usb-device-manager (and I already illustrated how) but after this commit<br>
> > is<br>
> > > done.<br>
> > > If from your point of view this is the requirement to existing commit,<br>
> > > please let me know (then we will need to reevaluate our plans and our<br>
> > > ability to deliver the patches in reasonable time).<br>
> ><br>
> > I'll quote again the comment:<br>
> ><br>
> >    /*<br>
> >     * On win32 we need to do this the hard and slow way, by asking libusb<br>
> > to<br>
> >     * re-enumerate all devices and then finding a matching device.<br>
> >     * We cannot cache the libusb_device like we do under Linux since the<br>
> >     * driver swap we do under windows invalidates the cached libdev.<br>
> >     */<br>
> ><br>
> > It says we cannot cache a libusb_device because it can change behind our<br>
> > back, so we need to reenumerate all devices and lookup the right<br>
> > libusb_device before every call to libusb.<br>
> > Before your changes, we did not hold any long-lasting references to a<br>
> > libusb_device on Windows, so we were guaranteed to do that lookup before<br>
> > every call. After your changes, SpiceUsbBackendDevice is holding a<br>
> > long-lasting reference to a libusb_device, but does not care about the<br>
> > win32 issue. Only usb-device-manager is making sure the libusb_device<br>
> > it's going to use is valid. This does not make sense to me to leak this<br>
> > implementation detail to usb-device-manager. SpiceUsbBackend should hide<br>
> > it, otherwise its API is going to be error-prone.<br>
> ><br>
> > When you say this can be simplified by removing this lookup, are you<br>
> > confirming that this comment I quoted above is obsolete?<br>
> ><br>
> <br>
> This comment, IMO, is wrong and permanent enumeration is not mandatory.<br>
> None of existing objects can disappear if it is is referenced.<br>
<br>
The libusb object does not disappear, but apparently there were some<br>
situations where the device 'moved', and the libusb object pointed to a<br>
no-longer existing device, thus the need to enumerate again.<br>
I believe this is related to<br>
<a href="https://gitlab.freedesktop.org/spice/spice-gtk/commit/76e94509cf29a78aa39740c81dcdd2eee355c7b9" rel="noreferrer" target="_blank">https://gitlab.freedesktop.org/spice/spice-gtk/commit/76e94509cf29a78aa39740c81dcdd2eee355c7b9</a><br>
<a href="https://bugzilla.redhat.com/show_bug.cgi?id=842816" rel="noreferrer" target="_blank">https://bugzilla.redhat.com/show_bug.cgi?id=842816</a></blockquote><div><br></div><div>This commit was discarder by further changes and related to some problem with WinUSB and there is</div><div>no other information about it. If the device is moved from one bus to another one, this is regular PnP event</div><div>and it should be processed as such.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Changes in existing patch are intentionally minimal to allow<br>
> comparison between previous and existing code. After the current patch<br>
> is merged I will be able to solve the problem of persistency as I<br>
> should be, I believe (together with unification of the structure<br>
> holding device properties). But mixing all these things together<br>
> discards the current patch and I can't predict the time frame of next<br>
> round.<br>
<br>
I'm not suggesting to squash everything in the current patch, but we are<br>
not limited to one single patch. If you intend to remove that<br>
persistency code right after the patch being discussed, it would help to<br>
send the 2 patches as a series.<br></blockquote><div><br></div><div>Even if I plan to do so, the removal of reenumeration is not something that</div><div>passed the same agressive testing as exising patch and I do not plan to bind</div><div>it with current patch. As current patch do not present any regression or</div><div>degradation, I suppose it does not request any changes in the infrastructure.</div><div>Of course, it is possible that existing API does not satisfy you. But it does not</div><div>look like this is the case.</div><div>Your suggestion is to move into 'backend' code part of the code from usb-device-manager</div><div>that anyway should be removed. This would create significant changes in the current patch</div><div>and require additional round of the tests. This is why I do not agree.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Regarding the unification of the structure holding device properties,<br>
since you are introducing the duplication in that patch, I'd rather that<br>
we don't add suboptimal code with vague promises that it's going to be<br>
fixed later.<br></blockquote><div><br></div><div>I do not know whuch 'vague promises' you mention.</div><div>Possible, this is not related to the matter.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
If you point me at a branch with all your patches applied including this<br>
one, I can try to help with some of the preparatory commits/some of the<br>
splitting.<br>
<br></blockquote><div><br></div><div>I've already sent you the illustration how this can be done.</div><div><div>See last 3 commits at <a href="https://gitlab.freedesktop.org/yuri_benditovich/spice-gtk/commits/pers-dev">https://gitlab.freedesktop.org/yuri_benditovich/spice-gtk/commits/pers-dev</a> </div></div><div>But this is NOT what I'm submitting right now.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Christophe<br>
</blockquote></div></div></div>