> On 28 Feb 2017, at 18:26, Frediano Ziglio <fzig...@redhat.com > <mailto:fzig...@redhat.com>> wrote: > >> >> On Tue, 2017-02-28 at 09:29 -0500, Frediano Ziglio wrote: >>>> >>>> Related: >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1373725 >>>> <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 <fzig...@redhat.com <mailto:fzig...@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.
I vote for the uint8_t version. There is no gain that I know of performance-wise with the uint32_t version. Christophe > > Frediano > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org> > https://lists.freedesktop.org/mailman/listinfo/spice-devel > <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
