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

[Christophe de Dinechin]

Frediano Ziglio writes:

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

Actually, I think the confusing was me inferring too much from the name
spice_usbredir_write_callback, see below.


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

This brings me back to my earlier question, why cannot you fall through, what
is the point of the "return 0"?

You replied "added a comment", but without sharing what the comment was,
so I still don't know ;-) Have you sent a follow-up patch that I missed?

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

Ah, I think I figured out what confused me. I assumed from the spice_
prefix that spice_usbredir_write_callback was a public API. So it did
not make sense to me to add some logic around it that would not be
done by the API itself. But spice_usbredir_write_callback is really just
an internal helper function that is neither public nor a callback, so it
does not really matter how you split the logic, it's invisible outside.


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

Actually, I was simply incorrectly assuming some other client could
refer to spice_usbredir_write_callback directly. I find it's not
appropriately named, since it's not really used as a callback, but
called directly, and because it has a "spice_" prefix that (to me)
implies it his "closer to the public API" than usbredir_write_callback,
when in fact it's further away from the public interface.


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


--
Cheers,
Christophe de Dinechin (IRC c3d)