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

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170301/a398bdda/attachment-0001.sig>