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

[Christophe de Dinechin]

Frediano Ziglio writes:

>>
>> 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?
>>
>
> Last patch I sent (maybe I sent later) has:
>
>     // handle first packet (HELLO) creating parser from capabilities
>     if (SPICE_UNLIKELY(ch->parser == NULL)) {
>         // Here we return 0 from this function to keep the packet in
>         // the queue. The packet will then be sent to the guest.
>         // We are initializing SpiceUsbBackendChannel, the
>         // SpiceUsbredirChannel is not ready to receive packets.
>
> that last "SpiceUsbredirChannel is not ready to receive packets" is
> the reason why not falling through.

Cool. Thanks for explaining. The comment really helps.

>
>> >
>> >>
>> >> > > 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.
>>
>
> I think the "spice_" prefix came from the fact that the class is public.
> Why "_callback" suffix I have no idea, maybe was used directly. Maybe
> is related to the fact the SpiceUsbredirChannel pointer in
> SpiceUsbBackendChannel is "user_data".

Maybe an opportunity for a later cleanup?

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


--
Cheers,
Christophe de Dinechin (IRC c3d)