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

[Jonathon Jongsma]

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)

> +{
> +    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.

> +}
> +
>  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'.

> +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);
>