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

[Christophe de Dinechin <cdupontd@xxxxxxxxxx>]

On 28 Feb 2017, at 18:26, Frediano Ziglio <fziglio@xxxxxxxxxx> 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.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel