[Spice-devel] [PATCH v2 1/3] spice-protocol: add vd_agent announce capabilities message
Alon Levy
alevy at redhat.com
Mon Aug 30 05:40:56 PDT 2010
----- "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
More information about the Spice-devel
mailing list