[Spice-devel] [spice-server 02/10] Rework red_channel_on_output a bit

[Frediano Ziglio]

> 
> On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> > Have the RedChannelClient callback call into a RedChannel callback
> > rather than doing the opposite. This will be useful in some
> > subsequent
> > refactoring of this code.
> > ---
> >  server/red-channel-client.c | 2 ++
> >  server/red-channel.c        | 9 ++-------
> >  server/red-channel.h        | 1 +
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-
> > client.c
> > index 0002951..82c786f 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -376,10 +376,12 @@ RedChannel*
> > red_channel_client_get_channel(RedChannelClient *rcc)
> >  void red_channel_client_on_output(void *opaque, int n)
> >  {
> >      RedChannelClient *rcc = opaque;
> > +    RedChannel *channel = red_channel_client_get_channel(rcc);
> >  
> >      if (rcc->priv->connectivity_monitor.timer) {
> >          rcc->priv->connectivity_monitor.out_bytes += n;
> >      }
> > +    red_channel_on_output(channel, n);
> >  }
> >  
> >  void red_channel_client_on_input(void *opaque, int n)
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index 835a744..92a22de 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -202,14 +202,9 @@ static void
> > red_channel_client_default_peer_on_error(RedChannelClient *rcc)
> >      red_channel_client_disconnect(rcc);
> >  }
> >  
> > -static void red_channel_on_output(void *opaque, int n)
> > +void red_channel_on_output(RedChannel *self, int n)
> >  {
> > -    RedChannelClient *rcc G_GNUC_UNUSED;
> > -    RedChannel *self G_GNUC_UNUSED;
> > -
> > -    red_channel_client_on_output(opaque, n);
> >  #ifdef RED_STATISTICS
> > -    self = red_channel_client_get_channel((RedChannelClient
> > *)opaque);
> >      stat_inc_counter(self->priv->reds, self->priv-
> > >out_bytes_counter, n);
> >  #endif
> >  }
> > @@ -344,7 +339,7 @@ red_channel_init(RedChannel *self)
> >      self->priv->outgoing_cb.on_error =
> >          (on_outgoing_error_proc)red_channel_client_default_peer_on_e
> > rror;
> >      self->priv->outgoing_cb.on_msg_done =
> > red_channel_client_on_out_msg_done;
> > -    self->priv->outgoing_cb.on_output = red_channel_on_output;
> > +    self->priv->outgoing_cb.on_output =
> > red_channel_client_on_output;
> >  
> >      self->priv->client_cbs.connect =
> > red_channel_client_default_connect;
> >      self->priv->client_cbs.disconnect =
> > red_channel_client_default_disconnect;
> > diff --git a/server/red-channel.h b/server/red-channel.h
> > index 7b6846f..291a00f 100644
> > --- a/server/red-channel.h
> > +++ b/server/red-channel.h
> > @@ -299,6 +299,7 @@ SpiceCoreInterfaceInternal*
> > red_channel_get_core_interface(RedChannel *channel);
> >  /* channel callback function */
> >  int red_channel_config_socket(RedChannel *self, RedChannelClient
> > *rcc);
> >  void red_channel_on_disconnect(RedChannel *self, RedChannelClient
> > *rcc);
> > +void red_channel_on_output(RedChannel *self, int n);
> >  void red_channel_send_item(RedChannel *self, RedChannelClient *rcc,
> > RedPipeItem *item);
> >  void red_channel_reset_thread_id(RedChannel *self);
> >  StatNodeRef red_channel_get_stat_node(RedChannel *channel);
> 
> 
> Although I think it's a little bit unfortunate that we need to expose
> another function in red-channel.h, it does seem to make more sense this
> way. The 'opaque' variable is a RedChannelClient[1], it makes more
> sense that this handler function is a client "method" rather than a
> channel method.
> 
> [1] speaking of which, why is it opaque instead of a RedChannelClient*?
> Maybe that will be addressed later in this series...
> 
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> 

About exposing red_channel_on_output.
The 2 classes are really coupled together. Why not adding a red-channel-private.h
storing function used just for this coupling? We could also prefix the
functions with underscore (or other way to distinguish).
Currently red_channel_on_output is just adding statistics. Why not moving
this counter to RedChannelClient ? RedChannelClient could use red_channel_get_stat_node
to get the node and add the counter by itself. Note that the counter would be shared
among all clients of the channel. Also statistic code are currently by default
compiled out.

Frediano