<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 16, 2019 at 6:11 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 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 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 device<br>
> properties.<br>
> <br>
> Now:<br>
> On Linux: usb-device-manager receives backend device wrapping libusb 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 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 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 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></blockquote><div><br></div><div>This comment, IMO, is wrong and permanent enumeration is not mandatory.</div><div>None of existing objects can disappear if it is is referenced.</div><div>But the object must be used with proper libusb context.</div><div><br></div><div>Changes in existing patch are intentionally minimal to allow comparison between</div><div>previous and existing code. After the current patch is merged I will be able to solve</div><div>the problem of persistency as I should be, I believe (together with unification of the</div><div>structure holding device properties). But mixing all these things together discards</div><div>the current patch and I can't predict the time frame of next round.</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">
<br>
Christophe<br>
</blockquote></div></div>