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

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

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

> 
> Regards,
> 
> Hans
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel