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

[Christophe de Dinechin]

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

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190828/13dc8e7d/attachment-0001.html>