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

[Christophe de Dinechin]

> On 28 Feb 2017, at 18:26, Frediano Ziglio <fziglio at redhat.com <mailto:fziglio at 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 <fziglio at redhat.com <mailto: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.

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 at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170301/92a9e19e/attachment.html>