On Fri, Mar 03, 2017 at 07:35:51AM -0500, Frediano Ziglio wrote: > > > > 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. VD_AGENT_SET_CAPABILITY is just a macro, so the patch adding VD_AGENT_CLEAR_CAPABILITY should make it a macro too. If there's a patch before or after that making VD_AGENT_SET_CAPABILITY use an inline func, then VD_AGENT_CLEAR_CAPABILITY should follow suit. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel