[Spice-devel] [spice-gtk v1 0/2] Add usb backend module

[Christophe Fergeau]

On Thu, Sep 27, 2018 at 06:56:12PM +0300, Yuri Benditovich wrote:
Hey,

> Christophe,
> 
> Our primary goal, if you remember, was to deliver cd-sharing feature.
> For that it was needed to isolate usb redirection modules from libusb and
> usbredir to create some abstraction layer for USB devices whether they are
> local or emulated in software.
> SpiceUsbDevice is type of object that Usb Device Manager uses internally,
> opaque for other modules and external API, with its own life cycle and with
> system dependent structure. It is referenced by external components (as
> widget or external application).
> Introduced by us SpiceUsbBackendDevice is used by abstraction layer
> internally, with its own life cycle and opaque for all other modules with
> system-independent structure.
> Current commit makes minimal changes in existing modules and serves the
> initial goal.

As you said, there's already a SpiceUsbDevice abstraction which is
opaque to code outside of usb-device-manager.c. Even
usb-device-manager.c is not heavily tied to it, as it mostly uses
accessors when it needs to interact with it.
Your work adds an additional SpiceUsbDeviceBackend abstraction, which
indeed abstracts some more SpiceUsbDevice by hiding the libusb_device
pointer. In doing so, it duplicates most of what SpiceUsbDevice already
has, I'd rather we don't duplicate too many things without a very good
reason..
Regarding the lifecycle when you take a look at channel-usbredir.c,
references on SpiceUsbDevice and SpiceUsbDeviceBackend are
ref'ed/unref'ed at the same time, so their lifecycles are not as
independant as one might think.

> Your suggestion breaks the initial goal and moves us to additional
> refactoring of already prepared 'engineering stable' code and additional
> complicated (IMO) reintegration with USB device manager.

The goal is both to have something which works, and something which is
maintainable upstream.. Limiting the amount of abstractions/duplicate
code we have to deal with definitely helps with the maintainability.

> This also breaks the original idea of isolation - such a way all the
> internals of 'backend device' will be visible to usb device manager,
> including libusb.

This does not sound like what I suggested:
« In short, I'd move SpiceUsbDeviceInfo definition from
usb-device-manager.c to a new usb-device.c (aka usb-device-backend.c) »
in other words, make SpiceUsbDevice opaque from usb-device-manager.c,
which would not make it much different from SpiceUsbDeviceBackend..


> From my point of view, the order should be following:
> 1. Finalize step 1 patches (isolation, opaque usb backend)
> 2. Add cd-sharing  feature above it
> 3. Finalize UI solution that serves cd-sharing
> 4. Reduce differences between Linux and Windows in Usb device manager
> (persistent backend device object)

Actually, whether we can have persistent backend device objects on
Windows or not impacts the isolation work, so it would be good to know
that before 1... If we need to 'renew' libusb_device pointers before
using them, then it would be better not to keep it in
SpiceUsbDeviceBackend when building for Windows (yes, exactly like what
is done for SpiceUsbDevice at the moment..)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180928/91d79f20/attachment.sig>