<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 28, 2018 at 12:59 PM Christophe Fergeau <<a href="mailto:cfergeau@redhat.com">cfergeau@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, Sep 27, 2018 at 06:56:12PM +0300, Yuri Benditovich wrote:<br>
Hey,<br>
<br>
> Christophe,<br>
> <br>
> Our primary goal, if you remember, was to deliver cd-sharing feature.<br>
> For that it was needed to isolate usb redirection modules from libusb and<br>
> usbredir to create some abstraction layer for USB devices whether they are<br>
> local or emulated in software.<br>
> SpiceUsbDevice is type of object that Usb Device Manager uses internally,<br>
> opaque for other modules and external API, with its own life cycle and with<br>
> system dependent structure. It is referenced by external components (as<br>
> widget or external application).<br>
> Introduced by us SpiceUsbBackendDevice is used by abstraction layer<br>
> internally, with its own life cycle and opaque for all other modules with<br>
> system-independent structure.<br>
> Current commit makes minimal changes in existing modules and serves the<br>
> initial goal.<br>
<br>
As you said, there's already a SpiceUsbDevice abstraction which is<br>
opaque to code outside of usb-device-manager.c. Even<br>
usb-device-manager.c is not heavily tied to it, as it mostly uses<br>
accessors when it needs to interact with it.<br>
Your work adds an additional SpiceUsbDeviceBackend abstraction, which<br>
indeed abstracts some more SpiceUsbDevice by hiding the libusb_device<br>
pointer. In doing so, it duplicates most of what SpiceUsbDevice already<br>
has, I'd rather we don't duplicate too many things without a very good<br>
reason..<br>
Regarding the lifecycle when you take a look at channel-usbredir.c,<br>
references on SpiceUsbDevice and SpiceUsbDeviceBackend are<br>
ref'ed/unref'ed at the same time, so their lifecycles are not as<br>
independant as one might think.<br>
<br>
> Your suggestion breaks the initial goal and moves us to additional<br>
> refactoring of already prepared 'engineering stable' code and additional<br>
> complicated (IMO) reintegration with USB device manager.<br>
<br>
The goal is both to have something which works, and something which is<br>
maintainable upstream.. Limiting the amount of abstractions/duplicate<br>
code we have to deal with definitely helps with the maintainability.<br>
<br>
> This also breaks the original idea of isolation - such a way all the<br>
> internals of 'backend device' will be visible to usb device manager,<br>
> including libusb.<br>
<br>
This does not sound like what I suggested:<br>
« In short, I'd move SpiceUsbDeviceInfo definition from<br>
usb-device-manager.c to a new usb-device.c (aka usb-device-backend.c) »<br>
in other words, make SpiceUsbDevice opaque from usb-device-manager.c,<br>
which would not make it much different from SpiceUsbDeviceBackend..<br>
<br>
<br>
> From my point of view, the order should be following:<br>
> 1. Finalize step 1 patches (isolation, opaque usb backend)<br>
> 2. Add cd-sharing  feature above it<br>
> 3. Finalize UI solution that serves cd-sharing<br>
> 4. Reduce differences between Linux and Windows in Usb device manager<br>
> (persistent backend device object)<br>
<br>
Actually, whether we can have persistent backend device objects on<br>
Windows or not impacts the isolation work, so it would be good to know<br>
that before 1... If we need to 'renew' libusb_device pointers before<br>
using them, then it would be better not to keep it in<br>
SpiceUsbDeviceBackend when building for Windows (yes, exactly like what<br>
is done for SpiceUsbDevice at the moment..)<br></blockquote><div><br></div><div>Know whether we can have persistent backend and libusb device on Windows means</div><div>preparing the POC that does it.</div><div> </div><div>Do you suggest different order of the changes in usb-redir than I've suggested?</div><div>If yes, can you please share it with us?</div><div><br></div><div>Thanks,</div><div>Yuri</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Christophe<br>
</blockquote></div></div>