[Spice-devel] [spice-server 2/8] Move RedChannel::data to ClientCbs::cbs_data

[Christophe Fergeau]

On Thu, Mar 24, 2016 at 12:21:05PM -0400, Frediano Ziglio wrote:
> > 
> > On Tue, Mar 15, 2016 at 05:11:36PM -0400, Frediano Ziglio wrote:
> > > > -static void red_channel_client_default_disconnect(RedChannelClient
> > > > *base)
> > > > +static void red_channel_client_default_disconnect(RedChannelClient
> > > > *base,
> > > > gpointer cbs_data)
> > > >  {
> > > >      red_channel_client_disconnect(base);
> > > >  }
> > > > @@ -1062,7 +1062,7 @@ RedChannel *red_channel_create(int size,
> > > >  
> > > >      client_cbs.connect = red_channel_client_default_connect;
> > > >      client_cbs.disconnect = red_channel_client_default_disconnect;
> > > > -    client_cbs.migrate = red_channel_client_default_migrate;
> > > > +    client_cbs.migrate =
> > > > (channel_client_migrate_proc)red_channel_client_default_migrate;
> > > > 
> > > 
> > > I don't like these kind of cast. Why don't you add a parameter (unused)
> > > to these functions?
> > 
> > red_channel_client_default_migrate() is not used only as a
> > ClientCbs::migrate vfunc, it's also called a few times in other code
> > (red_migrate_display() for example).
> > red_channel_client_default_migrate(rcc, NULL) did not look so great
> > there, and I prefer to have a cast rather than an intermediate function
> > with the right number of args calling directly the real function with
> > one arg less.
> > 
> > Christophe
> > 
> 
> I still don't like these casts.
> 
> What about this:
> https://cgit.freedesktop.org/~fziglio/spice-server/commit/?h=christophe&id=f3660395933ccd3198d59f52c02d6bb6f34788e7

Well, this is what I said in the previous email that I'm not a big fan
of ;)
« red_channel_client_default_migrate(rcc, NULL) did not look so
great there, and I prefer to have a cast rather than an intermediate
function with the right number of args calling directly the real
function with one arg less. »

> 
> Actually considering this function
> 
> static void red_channel_client_migrate_client(RedChannelClient *rcc)
> {
>     rcc->channel->client_cbs.migrate(rcc, rcc->channel->client_cbs.cbs_data);
> }
> 
> why not using rcc->channel->client_cbs.cbs_data inside the callback?
> Without the parameter change both subject and comment make sense so looks like
> adding the parameter is not necessary.

This assumes we are fine with the callback knowing about
RedChannelClient internals, and being able to access them. In my opinion
it's better if at least in the API, RedChannelClient could be a black
box.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160325/13076176/attachment.sig>