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

[Frediano Ziglio]

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

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

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