[Spice-devel] [PATCH] RedChannelClient: use Gobject properties

[Jonathon Jongsma]

On Mon, 2016-11-07 at 12:16 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Fri, Nov 04, 2016 at 11:19:22AM -0500, Jonathon Jongsma wrote:
> > > 
> > > Use g_param_spec_object() instead of g_param_spec_pointer() for
> > > the
> > > 'client' and 'channel' properties now that these types are
> > > GObjects.
> > > This improves refcounting and typesafety slightly.
> > > ---
> > >  server/red-channel-client.c | 32 ++++++++++++++++---------------
> > > -
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/server/red-channel-client.c b/server/red-channel-
> > > client.c
> > > index 84dd28f..ad4a545 100644
> > > --- a/server/red-channel-client.c
> > > +++ b/server/red-channel-client.c
> > > @@ -150,10 +150,10 @@ red_channel_client_get_property(GObject
> > > *object,
> > >              g_value_set_pointer(value, self->priv->stream);
> > >              break;
> > >          case PROP_CHANNEL:
> > > -            g_value_set_pointer(value, self->priv->channel);
> > > +            g_value_set_object(value, self->priv->channel);
> > 
> > If something is using getters, this will introduce a leak, but I
> > could
> > not find anything doing g_object_get(..., "channel", ...)
> > 


good point. I think everything currently uses the convenience function
red_channel_client_get_channel(), so they won't see any change due to
this patch. 

> 
> Probably this property can be write-only + construct so we don't
> need to get it?
> (not a regression by the way, just I noted that one of the last
> patch introduced such a kind of property)

I think it would be a bit weird to make this property write-only when
we provide a convenience function to get the property value
(red_channel_client_get_channel()). Perhaps the bigger question is
whether we should change the _get_channel() function to be have like
g_object_get() and adds a reference to the channel. that would
obviously require more work to change all callers to not leak.

> 
> > 
> > > 
> > >              break;
> > >          case PROP_CLIENT:
> > > -            g_value_set_pointer(value, self->priv->client);
> > > +            g_value_set_object(value, self->priv->client);
> > >              break;
> > >          case PROP_MONITOR_LATENCY:
> > >              g_value_set_boolean(value, self->priv-
> > > >monitor_latency);
> > > @@ -195,12 +195,10 @@ red_channel_client_set_property(GObject
> > > *object,
> > >          case PROP_CHANNEL:
> > >              if (self->priv->channel)
> > >                  g_object_unref(self->priv->channel);
> > > -            self->priv->channel = g_value_get_pointer(value);
> > > -            if (self->priv->channel)
> > > -                g_object_ref(self->priv->channel);
> > > +            self->priv->channel = g_value_dup_object(value);
> > >              break;
> > >          case PROP_CLIENT:
> > > -            self->priv->client = g_value_get_pointer(value);
> > > +            self->priv->client = g_value_get_object(value);
> > 
> > Just to be sure, we don't want to take a ref on the client?
> > 
> 
> Good question. More related to reference counting talk
> than this rationale.
> Currently behavior is if MainChannelClient is closed all clients
> (RedClient and others RedChannelClients) are destroyed.
> But for the other RedChannelClients a strong pointer to RedClient
> make sense. Not sure if RedClient currently has a strong pointer
> to MainChannelClient, surely a RedClient cannot exist without
> a MainChannelClient.

Yeah, it's difficult to say exactly what should happen here, but I
didn't want to change ownership as part of this patch. We probably
should review this as part of our other ownership discussions.

> 
> > 
> > Looks good to me as is.
> > 
> > Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> > 
> > Christophe
> > 
> 
> Agree
> 
> Frediano
>