[Spice-devel] [PATCH spice-protocol 2/2] agent: Add macro for clearing capability

[Frediano Ziglio]

> 
> On Tue, 2017-02-28 at 09:29 -0500, Frediano Ziglio wrote:
> > > 
> > > Related:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1373725
> > > ---
> > >  spice/vd_agent.h | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > index ac22498..3b1f183 100644
> > > --- a/spice/vd_agent.h
> > > +++ b/spice/vd_agent.h
> > > @@ -269,6 +269,9 @@ typedef struct SPICE_ATTR_PACKED
> > > VDAgentAnnounceCapabilities {
> > >  #define VD_AGENT_SET_CAPABILITY(caps, index) \
> > >      { (caps)[(index) / 32] |= (1 << ((index) % 32)); }
> > >  
> > > +#define VD_AGENT_CLEAR_CAPABILITY(caps, index) \
> > > +    { (caps)[(index) / 32] &= ~(1 << ((index) % 32)); }
> > > +
> > >  #include <spice/end-packed.h>
> > >  
> > >  #endif /* _H_VD_AGENT */
> > 
> > I would say
> > 
> > Acked-by: Frediano Ziglio <fziglio at redhat.com>
> > 
> > Honestly I don't think should be a 2/2, just a separate patch.
> > 
> > The related bug comment for a so generic patch looks a bit weird.
> true, sorry, it is a leftover
> > 
> > Would be sensible to have a static inline function instead of
> > a macro?
> 
> I did it as a complement to VD_AGENT_SET_CAPABILITY. Do you prefer a
> function because of the type check ? I don't mind adding it, but I'd
> keep something like:
> 
> static inline vd_agent_clear_capability(uint32_t *caps, uint32_t
> index);
> #define VD_AGENT_CLEAR_CAPABILITY vd_agent_set_capability
> 
> 

For me make sense. Let's see what other people think (if they prefer
same style).

About capabilities and how to save. Currently the code uses little
endian but as we are moving to support also big endian platforms
we are adding code to swap the uint32_t. If we assume little
endian these expression produce the same results:

   uint32_t *caps;
   ...
   caps[i / 32] |= 1 << (i % 32);

   uint32_t *caps;
   ...
   ((uint8_t*)caps)[i / 8] |= 1 << (i % 8);

But this last one does not require to swap to/from network in
case of big endian.
Could we use a different structure to store capabilities
to avoiding swapping the array?
Would make code to support big endian easier.

Frediano