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@xxxxxxxxxx> > > > > Christophe > > > > Agree > > Frediano > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel