[Spice-devel] [PATCH spice-protocol 2/2] agent: Add macro for clearing capability
Christophe de Dinechin
cdupontd at redhat.com
Wed Mar 1 13:31:52 UTC 2017
> 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>
More information about the Spice-devel
mailing list