[Spice-devel] [PATCH spice-server 3/6] red-channel: Add red_channel_pipes_add function

[Frediano Ziglio]

> 
> On Tue, 2017-08-29 at 11:53 +0100, Frediano Ziglio wrote:
> > Considering that now RedPipeItem have reference counting
> > and that lot of items are just used to store constant
> > data to send, using reference counting instead of creating
> > different items for each client is easier to do.
> > So this new red_channel_pipes_add allows to add a single item
> > to all clients.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/red-channel.c | 12 ++++++++++++
> >  server/red-channel.h |  2 ++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 9ff3474a..6e8e9c6b 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -430,6 +430,18 @@ void
> > red_channel_init_outgoing_messages_window(RedChannel *channel)
> >                     (GFunc)red_channel_client_init_outgoing_messages_
> > window, NULL);
> >  }
> >  
> > +void red_channel_pipes_add(RedChannel *channel, RedPipeItem *item)
> 
> I was wondering if it would be useful to make it more obvious by the
> name that the item would be shared between clients (e.g.
> red_channel_pipes_add_shared() or _add_shared_item())? Maybe that's
> overkill and we can just handle it with an API comment (see below)
> 

I would go for a comment.
Suggestion welcome.

> > +{
> > +    RedChannelClient *rcc;
> > +
> > +    FOREACH_CLIENT(channel, rcc) {
> > +        red_pipe_item_ref(item);
> > +        red_channel_client_pipe_add(rcc, item);
> > +    }
> > +
> > +    red_pipe_item_unref(item);
> 
> I'm a little bit ambivalent about this part. On the one hand, it's more
> convenient for the caller since they dont' have to explicitly unref the
> pipe item. But it also means that the pipe item might no longer be
> valid after this function returns, which might be a little unexpected?
> As long as it's documented in the header, I guess it's OK.
> 

All the XXX_pipe(s)_add_YYY functions take ownership of the
item.
This does not mean should not documented but then should be
documented for all of them.

> > +}
> > +
> >  static void red_channel_client_pipe_add_type_proxy(gpointer data,
> > gpointer user_data)
> >  {
> >      int type = GPOINTER_TO_INT(user_data);
> > diff --git a/server/red-channel.h b/server/red-channel.h
> > index e65eea1e..c965baee 100644
> > --- a/server/red-channel.h
> > +++ b/server/red-channel.h
> > @@ -174,6 +174,8 @@ void red_channel_pipes_add_type(RedChannel
> > *channel, int pipe_item_type);
> >  
> >  void red_channel_pipes_add_empty_msg(RedChannel *channel, int
> > msg_type);
> >  
> 
> 
> I think it would be useful to add a comment here indicating that 'item'
> will be shared between all clients. Also should specify that it takes
> ownership of 'item'.
> 

Same here about ownership.
Why should be documented that the item is shared? The only issue you
could have is that the item is changed while you process it.
But is processed by RedChannelClient which know how to handle it
and RedChannelClient should not call red_channel_pipes_add_empty_msg.

> > +void red_channel_pipes_add(RedChannel *channel, RedPipeItem *item);
> > +
> >  /* return TRUE if all of the connected clients to this channel are
> > blocked */
> >  bool red_channel_all_blocked(RedChannel *channel);
> >  
> 

Frediano