On 28 Aug 2019, at 12:16, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:Side comment: usbredir_write_callback used to be a mere wrapper aroundspice_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 betweenusbredir_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 usedwhere 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 thatusbredir_write_callback is no longer a simple wrapper, it mayindicate that additional comments are required to explain whateach 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.
// 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 someboundary is subtly moving between the components. But I’m notreally familiar enough with usbredir to understand the intent just fromthe patches ;-)
Mainly before the flow was a single one (guest <-> usbredirhost), now data can flow
to/from the "parser" to support emulated devices.
ThanksChristophe
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel