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

[Christophe de Dinechin <christophe.de.dinechin@xxxxxxxxx>]

On 28 Aug 2019, at 12:16, Frediano Ziglio <fziglio@xxxxxxxxxx> 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?

In what context would spice_usbredir_write_callback be used

where the new logic in usbredir_write_callback is not necessary?

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

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

Thanks

Christophe

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel