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

[Frediano Ziglio]

> 
> On Tue, Feb 28, 2017 at 12:26:41PM -0500, Frediano Ziglio wrote:
> > > 
> > > 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.
> 
> VDAgentAnnounceCapabilities currently is defined as
> 
> typedef struct SPICE_ATTR_PACKED VDAgentAnnounceCapabilities {
>     uint32_t request;
>     uint32_t caps[0];
> } VDAgentAnnounceCapabilities;
> 
> Not swapping 'caps' which is an array of uint32_t would look odd imo.
> There's also a SpiceMainChannel::agent-caps-0 property which is an int,
> not exactly sure what the expected behaviour is for this one..
> 
> Not strictly opposed to your proposal, just don't want to try to be too
> smart if that leads to code which is harder to follow.
> 
> Christophe
> 

I should had probably put some comment on this "BYTE" change.
I didn't mean to stop the initial patch with this proposal
but just to add a comment on one though I had.
Moving to a different "storage" is out of the scope of initial patch,
the main question about the patch was using inline+macro instead
of just macro.

Probably better to discuss on possible storage change on another
thread.

Frediano