[Spice-devel] [PATCH spice-gtk v4 24/29] usb-backend: Rewrite USB emulation support

[Christophe de Dinechin]

Preamble: Apologies for screwing up the formatting so badly in my last
reply. Trying to repair a little manually here.

Frediano Ziglio writes:

> On 28 Aug 2019, at 12:16, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > I have the feeling I didn't get what you wanted to say.
>
> > I meant: what is the separation of concerns between
> > usbredir_write_callback and spice_usbredir_write_callback?
> ABI. usbredir_write_callback is a callback for usbredir layer, spice_usbredir_write_callback
> expects a SpiceUsbredirChannel.

Well, this is not really a "separation of concerns" explanation, more
like information about how the implementation evolved. My question was
more about what responsibility each component had.

That does not invalidate your answer, which I read as "it's not
practical to change the ABI for that change". But I think that
the real answer to my question is at the end, please check if I
understood correctly.


> > In what context would spice_usbredir_write_callback be used
> > where the new logic in usbredir_write_callback is not necessary?
>
> You need to adjust to the ABI of the two. spice_usbredir_write_callback is supposed
> to write the packet to the guest (or handle the packet anyway).

Yes, but I'm trying to understand what happens for example if there is a
version mismatch. Who deals with the HELLO message?


> The new logic (added by Yuri patch, not changed here by this patch) is here to handle
> the initial HELLO packet.

True, but it's while reviewing your patch that I noticed all the added
logic and tried to understand where this was going, hence my questions
;-)


> > If the way you organized the code is somehow better, given that
> > usbredir_write_callback is no longer a simple wrapper, it may
> > indicate that additional comments are required to explain what
> > each does. Or maybe it's perfectly clear to everyone but me ;-)

> I suppose the "handle first packet (HELLO) creating parser from capabilities"
> is not enough. Would:
>
>     // Handle first packet (HELLO) creating parser from capabilities.
>     // If we are initializing and we don't have the parser we get the
>     // capabilities from the usbredirhost and use them to initialize
>     // the parser.
>
> be better?

It's slightly better, but I was more thinking about adding (later) a
comment to spice_usbredir_write_callback, in order to indicate that it
does NOT deal with the HELLO packet, i.e. that the data processing logic
is split between the two layers.


> > Hmmm. That tends to confirm the impression above that some
> > boundary is subtly moving between the components. But I'm not
> > really familiar enough with usbredir to understand the intent just from
> > the patches ;-)

> Mainly before the flow was a single one (guest <-> usbredirhost), now data can flow
> to/from the "parser" to support emulated devices.

I think that it's "to support emulated devices" that is really important
here. In terms of separation of concerns, that would mean that the
library does not care about that, and that only the GTK client provides the
logic for emulating devices. Is that a good way to describe it?

Would another theoretical client using the library never run into the
case because it would not expose the capabilities? Or is it just a case
that nobody cares about?

(Also, to be clear, I don't think this invalidates the patch at all, I'm
really asking questions to make sure I understand the logic and that the
way the code is organized is fully intentional).

--
Cheers,
Christophe de Dinechin (IRC c3d)