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

[Jonathon Jongsma]

On Tue, 2017-08-29 at 11:49 -0400, Frediano Ziglio wrote:
> > 
> > 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_m
> > > > > essa
> > > > > 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

Sure, thanks. just change "client" to "clients" in the first sentence
;)

> 
> > > 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