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