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

[Frediano Ziglio]

> 
> 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().
> 

The fact that other language have this or self does not mean we should
have a this in C. If we want C++ ABI we can move to C++.
You are changing RedClient and so red_client_ prefix make sense on the
other way you are passing a pointer to RedClient, it's inside RedChannelClient.
C does not provide either the encapsulation of other OO language nor the
ABI.
Usually (say 99%) providing a direct pointer is the right way, I think
this function is an exception.
Probably in C++ I would have implemented RedChannelClient constructor
with a RedClient reference and the removal automatically in the destructor.

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