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

[Frediano Ziglio]

> 
> On Tue, 2017-08-29 at 11:14 -0400, Frediano Ziglio wrote:
> > > 
> > > 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_messa
> > > > ges_
> > > > 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.
> 
> Well, that's true for the client functions, but not really for the
> channel ones. The old red_channel_pipes_* functions actually created
> the item add added it to the clients. The caller had no reference to
> the item so there was no possibility of a dangling pointer. Now the
> caller is creating the item itself and passing it to the function,
> which is potentially freeing it. But maybe I'm being overly cautious?
> 

Added a

/* Add an item to all the client connected.
 * The same item is shared between all clients.
 * Function will take ownership of the item.
 */

I won't send an update series for this change

> > 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.
> 
> Yeah, maybe there's no real need to document it. But for many years the
> code created separate pipe items for each client. Now the behavior
> changed, so I thought it would be good to document that. Maybe not
> necessary.
> 
> > 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