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

[Frediano Ziglio]

> On 28 Aug 2019, at 12:16, Frediano Ziglio < fziglio at redhat.com > wrote:

> > > Side comment: usbredir_write_callback used to be a mere wrapper around
> > 
> 
> > > spice_usbredir_write_callback. Now, it has a whole lot of logic in it.
> > 
> 
> > > Is it intentional, or should some of that logic be moved to shared code?
> > 
> 

> > Yes, intentional (otherwise why changing the code?).
> 
> > This code is not shared with anything. The only reason to put in a separate
> 
> > function is separation, not sharing.
> 
> > 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. 

> 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). 
The new logic (added by Yuri patch, not changed here by this patch) is here to handle 
the initial HELLO packet. 

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

> This may also be intended for some follow-up patches?

> […]

> > There are still some minor weirdness in the initial patch.
> 
> > Like why usbredir_hello is called with a NULL parameter instead of having
> 
> > a separate "initialize_edev" or similar.
> 
> > Or why parser code calls a lot usbredir_write_flush_callback which was
> 
> > previously called only by usbredirhost and is supposed to dispatch between
> 
> > usbredirhost or parser why from the parser is called only to flush from
> 
> > parser if channel is ready.
> 

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

> Thanks
> Christophe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190828/d8366441/attachment.html>