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

[Hans de Goede]

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.

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