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

[Hans de Goede]

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 ?

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

Why the size field here, VDAgentMessage already has a size field
which can be used. On second thought, lets keep this see below.

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

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

Regards,

Hans