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?
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel