[Spice-devel] [PATCH v2 1/3] spice-protocol: add vd_agent announce capabilities message

[Alon Levy]

----- "Hans de Goede" <hdegoede at redhat.com> wrote:

> Hi,
> 
> On 08/30/2010 02:40 PM, Alon Levy wrote:
> >
> > ----- "Hans de Goede"<hdegoede at redhat.com>  wrote:
> >
> >> Hi,
> >>
> >> On 08/30/2010 01:51 PM, Alon Levy wrote:
> >>> Announce capabilities message for agent/client.
> >>>
> >>> v2:
> >>>    - variable length field
> >>>    - includes request field, true means receiver should respond
> with
> >> same but with false in request.
> >>>    - client sends with true if it sees agent already connected on
> >> startup, otherwise sends with false
> >>
> >> This smells like a race to me, why not simply always send with
> request
> >> true, like in the agent ?
> >>
> >
> > I guess that's simpler. Wanted to use the information I have in the
> client, namely
> > if the guest is already connected then we may miss the
> announce_capabilities from
> > it, so request=true, but if we catch the agent connecting, then we
> surely can't miss
> > it's announce_capabilities, so request=false. I don't think there is
> a race here. But
> > I can make it so anyway (it will cause two messages to be sent
> sometimes, triggering
> > double the send_display for instance).
> >
> 
> Looking at the implementation (after reviewing this patch), I think
> what you have
> is fine, I should have mentioned that in my other review.
> 
> >>>    - agent always sends initial with true.
> >>>    - removed capability enum, message types are capabilities
> >>>    - added VD_AGENT_END_MESSAGE
> >>>    - added VD_AGENT_CAPS_SIZE and VD_AGENT_CAPS_BYTES helpers
> >>>    - fixed VD_AGENT_HAS_CAPABILITY and VD_AGENT_SET_CAPABILITY
> >>>    - both client and agent now store the capabilities (in
> RedClient
> >> and VDAgent instances)
> >>>
> >>> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> >>> index 1fcda88..74c955f 100644
> >>> --- a/spice/vd_agent.h
> >>> +++ b/spice/vd_agent.h
> >>> @@ -64,6 +64,8 @@ enum {
> >>>        VD_AGENT_REPLY,
> >>>        VD_AGENT_CLIPBOARD,
> >>>        VD_AGENT_DISPLAY_CONFIG,
> >>> +    VD_AGENT_ANNOUNCE_CAPABILITIES,
> >>> +    VD_AGENT_END_MESSAGE,
> >>>    };
> >>>
> >>>    typedef struct SPICE_ATTR_PACKED VDAgentMonConfig {
> >>> @@ -129,6 +131,21 @@ enum {
> >>>        VD_AGENT_CLIPBOARD_UTF8_TEXT = 1,
> >>>    };
> >>>
> >>> +typedef struct SPICE_ATTR_PACKED VDAgentAnnounceCapabilities {
> >>> +    uint8_t  request;
> >>
> >> Please make this an uint32_t, making this an uint8_t combined with
> >> SPICE_ATTR_PACKED will make caps_size and caps not be 32 bit
> aligned,
> >> causing performancy penalties on Intel, and segfaults / bus errors
> >> on non Intel architectures (like the arm used in android). All
> cpu's
> >> but intel do *not* allow non word aligned addresses for word
> >> accesses.
> >>
> >>> +    uint32_t caps_size;
> >>
> > I'm missing the point of having a packed struct if we don't allow
> > stuff like this. unless of course I put the request at the end. And
> > for this case it's pretty pointless to haggle over three bytes I
> guess.
> > I don't want to put it in the end since it makes computing it's
> location trickier.
> >
> 
> Well I'm note the one who decided to use packed structs all over
> the place in spice :). More seriously though we (well someone) needs
> to
> take a good look at all spice protocol stuff wrt portability. For
> example
> I'm pretty sure that spice client won't work on ppc machines, as
> everything
> just assumes that taking a block of memory from the wire and
> interpreting
> it as an int is fine. Which is wrong when dealing with intel on one
> side and ppc on the other.
> 
> My comment here that request should be an int32 is to make avoid any
> future
> problems, which we might otherwise get with it. AFAIK at least spice
> client
> should be portable to as many archs as possible and thus we should
> keep
> things like endianness and alignment in mind when adding protocol
> extensions.
> 

Regarding the spice protocol not including the vdagent subprotocol, we
now have different on wire and in memory structures, and the in memory
should not be packed, avoiding all of those issues (and the marshalling/
demarshalling code should of course be portable, not sure if it is atm).

> >> Why the size field here, VDAgentMessage already has a size field
> >> which can be used. On second thought, lets keep this see below.
> >>
> >
> > Actually I think you're right, I should remove it.
> >
> >>> +    uint32_t caps[0];
> >>> +} VDAgentAnnounceCapabilities;
> >>> +
> >>> +#define VD_AGENT_CAPS_SIZE ((VD_AGENT_END_MESSAGE + 31) / 32)
> >>> +
> >>> +#define VD_AGENT_CAPS_BYTES (((VD_AGENT_END_MESSAGE + 31) / 8)&
> >> (0xffffffff - 3))
> >>> +
> >>
> >> Why not just (VD_AGENT_CAPS_SIZE * sizeof(uint32_t)) ? At least
> write
> >> the mask as ~3 for readability.
> >>
> > sorry for that, I wanted to make sure no compiler optimization made
> it
> > an incorrect result. I'll use the ~3.
> >
> >>> +#define VD_AGENT_HAS_CAPABILITY(caps, caps_size, index) \
> >>> +    ((index)<   (caps_size * 32)&&   ((caps)[(index) / 32]&  
> (1<<
> >> ((index)&   0x1f))))
> >>> +
> >>> +#define VD_AGENT_SET_CAPABILITY(caps, index) \
> >>> +    { (caps)[(index) / 32] |= (1<<   ((index)&   0x1f)); }
> >>>
> >>
> >> Hmm, okay so I'm starting to see sense in having a caps_size
> field.
> >> in
> >> VDAgentAnnounceCapabilities. Can we make these macro take an
> entire
> >> VDAgentAnnounceCapabilities struct as argument, that would make
> >> the use of VD_AGENT_HAS_CAPABILITY a bit easier to read.
> >
> > I prefer keeping it untied to the struct (so it can be used on any
> > array of uint32_t, like VDgent::_client_caps or
> RedClient::_agent_caps).
> >
> 
> I understand, that is fine.
> 
> Regards,
> 
> Hans