[Spice-devel] [RFC v4 18/62] server/red_channel: introduce pipes functions

[Alon Levy]

On Tue, May 03, 2011 at 01:52:38AM +0200, Marc-André Lureau wrote:
> On Tue, Apr 26, 2011 at 12:54 PM, Alon Levy <alevy at redhat.com> wrote:
> > Introduce functions to add (via producer method) the same item to multiple
> > pipes, all for the same channel.
> >
> > Note: Right now there is only a single channel, but the next patches will do the
> > per-channel breakdown to channel and channel_client before actually introducing
> > a ring in RedChannel, this makes it easier to make smaller changes - the
> > channel->rcc link will exist until removed in the ring introducing patch.
> > ---
> >  server/red_channel.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  server/red_channel.h |   12 ++++++++++-
> >  2 files changed, 62 insertions(+), 1 deletions(-)
> >
> > diff --git a/server/red_channel.c b/server/red_channel.c
> > index aff99ed..7e1edbe 100644
> > --- a/server/red_channel.c
> > +++ b/server/red_channel.c
> > @@ -1051,4 +1051,55 @@ void red_client_set_main(RedClient *client, MainChannelClient *mcc) {
> >     client->mcc = mcc;
> >  }
> >
> > +/*
> > + * Functions to push the same item to multiple pipes.
> > + */
> > +
> > +/*
> > + * TODO: after convinced of correctness, add paths for single client
> > + * that avoid the whole loop. perhaps even have a function pointer table
> > + * later.
> > + * TODO - inline? macro? right now this is the simplest from code amount
> > + */
> > +
> > +typedef void (*rcc_item_t)(RedChannelClient *rcc, PipeItem *item);
> > +typedef int (*rcc_item_cond_t)(RedChannelClient *rcc, PipeItem *item);
> >
> > +void __red_channel_pipes_create_batch(RedChannel *channel,
> > +                                new_pipe_item_t creator, void *data,
> > +                                rcc_item_t visitor)
> 
> The "__" prefix is not necessary imho, however a static modifier would be nice.
> 
> As you know already, I am not fond of the "visitor" name, since here
> it's just a callback, and not a real visitor "object" (ie with a state
> and multiple visit() methods etc..).
> 
> > +{
> > +    RedChannelClient *rcc;
> > +    PipeItem *item;
> > +    int num = 0;
> > +
> > +    if (!(rcc = channel->rcc)) {
> > +        return;
> > +    }
> > +    item = (*creator)(rcc, data, num++);
> > +    if (visitor) {
> > +        (*visitor)(rcc, item);
> > +    }
> > +}
> > +
> > +void red_channel_pipes_new_add_push(RedChannel *channel,
> > +                              new_pipe_item_t creator, void *data)
> > +{
> > +    __red_channel_pipes_create_batch(channel, creator, data,
> > +                                     red_channel_client_pipe_add);
> > +    red_channel_push(channel);
> > +}
> > +
> > +void red_channel_pipes_new_add(RedChannel *channel, new_pipe_item_t creator, void *data)
> > +{
> > +    __red_channel_pipes_create_batch(channel, creator, data,
> > +                                     red_channel_client_pipe_add);
> > +}
> > +
> > +// despite the name, this should push (TODO: match function to name)
> 
> yeah... that could be fixed easily instead of left as a TODO. No?
> 
> Imho, the responsibility to call push() should be left to the caller instead...
> 
> > +void red_channel_pipes_new_add_tail(RedChannel *channel, new_pipe_item_t creator, void *data)
> > +{
> > +    __red_channel_pipes_create_batch(channel, creator, data,
> > +                                     red_channel_client_pipe_add_tail_no_push);
> > +    red_channel_push(channel);
> > +}
> 
> > diff --git a/server/red_channel.h b/server/red_channel.h
> > index 877c567..15d70b6 100644
> > --- a/server/red_channel.h
> > +++ b/server/red_channel.h
> > @@ -271,6 +271,16 @@ void red_channel_client_set_message_serial(RedChannelClient *channel, uint64_t);
> >  void red_channel_client_begin_send_message(RedChannelClient *rcc);
> >
> >  void red_channel_pipe_item_init(RedChannel *channel, PipeItem *item, int type);
> > +
> > +// TODO: add back the channel_pipe_add functionality - by adding reference counting
> > +// to the PipeItem.
> > +
> > +// helper to push a new item to all channels
> > +typedef PipeItem *(*new_pipe_item_t)(RedChannelClient *rcc, void *data, int num);
> 
> What is "num" doing?
> 

This is meant to be used by the new function when adding to multiple pipes. The num
goes from 0 to n-1 when adding to n pipes (for a channel with n clients).

> Would it be more future proof to take PipeItem * as argument, that can
> be either referenced or copied by the various pipes?
I guess that's possible, so it would be NULL for the first and non NULL for the rest?
sounds like it would work for the only use case I had for num (where I tested num!=0).

> (see general comment in 00/62 reply about sharing PipeItem for various pipes)
> 
See my response to same.

> > +void red_channel_pipes_new_add_push(RedChannel *channel, new_pipe_item_t creator, void *data);
> > +void red_channel_pipes_new_add(RedChannel *channel, new_pipe_item_t creator, void *data);
> > +void red_channel_pipes_new_add_tail(RedChannel *channel, new_pipe_item_t creator, void *data);
> > +
> >  void red_channel_client_pipe_add_push(RedChannelClient *rcc, PipeItem *item);
> >  void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item);
> >  void red_channel_client_pipe_add_after(RedChannelClient *rcc, PipeItem *item, PipeItem *pos);
> > @@ -374,4 +384,4 @@ void red_client_destroy(RedClient *client);
> >  void red_client_set_main(RedClient *client, MainChannelClient *mcc);
> >  MainChannelClient *red_client_get_main(RedClient *client);
> >
> > -#endif
> > +#endif
> > \ No newline at end of file
> > --
> > 1.7.4.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau