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

[Christophe Fergeau]

On Fri, Sep 28, 2018 at 02:49:07PM +0300, Yuri Benditovich wrote:
> On Fri, Sep 28, 2018 at 12:59 PM Christophe Fergeau <cfergeau at redhat.com>
> wrote:
> 
> > 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..)
> >
> 
> Know whether we can have persistent backend and libusb device on Windows
> means preparing the POC that does it.

"Preparing the POC" should just be removing a bunch of #ifdef
G_OS_WIN32 from usb-device-manager.c, not a lot more. Hopefully Uri will
remember why this was added, and have a definite answer whether this is
still needed or not with UsbDK and the removal of UsbClerk :)

> Do you suggest different order of the changes in usb-redir than I've
> suggested?
> If yes, can you please share it with us?

I mostly agree, though we probably disagree on what 1. means exactly ;)
Regarding 4., if we get a quick answer whether this is possible or not,
I'd try to do it early, as this means the rest of the series will be
simpler..

Christophe

> 
> Thanks,
> Yuri
> 
> 
> 
> >
> > 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/b03b39b8/attachment.sig>