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.
But the object must be used with proper libusb context.
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.
Christophe
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel