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

[Frediano Ziglio]

> 
> 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.
> 

Oh... now I understand the confusion (I hope).
The if code returns always 0. So the message is not removed from the queue.
On the next call parser won't be NULL and the same HELLO packet will be
send to spice_usbredir_write_callback.
May be the "handle" in the comment is suggesting that the if code is also
consuming entirely the packet. Suggestions to improve the comment?

> 
> > > 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?
> 

The code is in spice-client-glib so not technically only GTK, all clients
that will use spice-client-glib. This "emulation" is transparent, for the
guest is like a remote physical device.

> 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?
> 

I don't understand. Do you mean the capabilities inside the HELLO packet?
In this case it's just the confusion about the "handle" above, the HELLO
is sent to the guest.

> (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)
> 

Frediano