[Spice-devel] [PATCH 04/18] Make red_client_remove_channel() a RedClient method

[Jonathon Jongsma]

On Thu, 2016-04-28 at 13:09 -0400, Frediano Ziglio wrote:
> > 
> > Explicitly pass RedClient* as first argument to this function, to mirror
> > red_client_add_channel(). This will eventually allow us to separate the
> > implementations of RedClient and RedChannelClient so that they don't
> > need to be poking into the internals of each other's structs.
> 
> I quite disagree. Unless you could remove client field from RedChannelClient
> I don't see the point of having red_client_add_channel and
> red_client_remove_channel having the same prototype.

Fair enough. I think added that because it seemed that this function was
intended to be a method of RedClient. And since methods in C don't automatically
provide a 'this' pointer, the standard way to mimic that is to provide a 'self'
parameter as the first argument of the function. If we don't add this parameter
to the function, I think that we should change the function name to signify that
it is not a RedClient method. Perhaps red_channel_client_remove().


> 
> Frediano
> 
> > ---
> >  server/red-channel.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index fbe00fc..2bcf919 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -74,7 +74,7 @@ static void
> > red_channel_client_restart_ping_timer(RedChannelClient *rcc);
> >  
> >  static void red_channel_client_event(int fd, int event, void *data);
> >  static void red_client_add_channel(RedClient *client, RedChannelClient
> >  *rcc);
> > -static void red_client_remove_channel(RedChannelClient *rcc);
> > +static void red_client_remove_channel(RedClient *client, RedChannelClient
> > *rcc);
> >  static RedChannelClient *red_client_get_channel(RedClient *client, int
> > type,
> >  int id);
> >  static void red_channel_client_restore_main_sender(RedChannelClient *rcc);
> >  static inline int red_channel_client_waiting_for_ack(RedChannelClient
> > *rcc);
> > @@ -1271,7 +1271,7 @@ void red_channel_client_destroy(RedChannelClient *rcc)
> >  {
> >      rcc->destroying = 1;
> >      red_channel_client_disconnect(rcc);
> > -    red_client_remove_channel(rcc);
> > +    red_client_remove_channel(rcc->client, rcc);
> >      red_channel_client_unref(rcc);
> >  }
> >  
> > @@ -1822,12 +1822,13 @@ static void
> > red_channel_remove_client(RedChannelClient *rcc)
> >      // TODO: should we set rcc->channel to NULL???
> >  }
> >  
> > -static void red_client_remove_channel(RedChannelClient *rcc)
> > +static void red_client_remove_channel(RedClient *client, RedChannelClient
> > *rcc)
> >  {
> > -    pthread_mutex_lock(&rcc->client->lock);
> > +    g_return_if_fail(client == rcc->client);
> > +    pthread_mutex_lock(&client->lock);
> >      ring_remove(&rcc->client_link);
> > -    rcc->client->channels_num--;
> > -    pthread_mutex_unlock(&rcc->client->lock);
> > +    client->channels_num--;
> > +    pthread_mutex_unlock(&client->lock);
> >  }
> >  
> >  static void red_channel_client_disconnect_dummy(RedChannelClient *rcc)
> > --
> > 2.4.11
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >