[Spice-devel] [PATCH spice-server 1a/3] red-channel: Use directly a GArray to pass capabilities

[Frediano Ziglio]

> 
> On Tue, Feb 28, 2017 at 12:14:25PM -0500, Frediano Ziglio wrote:
> > > 
> > > On Tue, Feb 28, 2017 at 09:43:33AM +0000, Frediano Ziglio wrote:
> > > > Capabilities where almost always passed using 2 arguments,
> > > > a number of elements and an array but then before using
> > > > these were converted to a GArray.
> > > > Converting to GArray much earlier allows to easily pass
> > > > the capabilities around.
> > > 
> > > Hey, nice improvement!
> > > 
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > > I don't know if is worth defining an abstract type
> > > > instead of using a simple GArray.
> > > > Maybe a "typedef GArray RedCapabilities" could help future
> > > > replacement of the GArray?
> > > 
> > > There's already a RedChannelCapabilities in red-channel.h, I'd reuse
> > > this one.
> > > 
> > 
> > Mumble... contains both common_caps and normal ones but can
> > be done, we always pass them together.
> > I was thinking about just joining caps+num.
> 
> Yeah, I know, but during review I came across that preexisting type.
> Since that type is already there, imo it's even cleaner to pass a single
> set of caps rather than always passing the 2 arrays.
> 
> > 
> > > > On the other side this patch moves most of the array boxing
> > > > into a single function.
> > > 
> > > I'd create the GArray one level higher, in reds_channel_do_link() rather
> > > than red_channel_connect, let reds.c do the grunt work, and have
> > > RedChannel deal with higher level types.
> > > 
> > > Christophe
> > > 
> > 
> > If you want to reuse RedChannelCapabilities we could just box this
> > type and avoid the GArray change.
> > Or use GArray in a follow up (or not!).
> > Other work for reds.c? Is not enough the cacophonous stuff in here?
> 
> I think I'd change RedChannelCapabilities to store 2 GArrays and pass
> that around. As RedChannel properties, it's probably easier to keep 2
> different GArrays (?)
> 
> Christophe
> 

I missed tha main channel so now the patch make stuff even better,
https://cgit.freedesktop.org/~fziglio/spice-server/commit/?h=common_graphics_channel_client&id=9ae24c5bf2528e6debf3465ec8760983007ea108

About possible improves I'm quite of the opinion that would be
better to have a follow up.
Changing RedChannelCapabilities would require lot of additional
changes here and there and I would then prefer to have also a
single property. Arriving till the end to have to split the
structure for 2 properties doesn't seems really good.
Looking at all paths common+channel caps arrive from client in
the link message (the first raw message from the client) and
are just forwarded together (previously as 4 parameters), then
converted to 2 GArrays then converted back to common+channel caps.
Looking at all path would make more sense to have always together.
I would box the RedChannelCapabilities.

Unfortunately 3/3 depends on this.
I'll update this and add a follow up with the boxing of current
structure.

Frediano