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

[Jonathon Jongsma]

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?

> 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