> > 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@xxxxxxxxxx> > > > > > > > > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel