[Bug 29458] TLS-connection branch

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29458

--- Comment #4 from Cosimo Cecchi <cosimoc at gnome.org> 2010-08-19 03:12:53 PDT ---
(In reply to comment #3)

Simon, thanks for the review. I now fixed my branch according to your comments
in separate commits.
I also found another bug in what was the last commit of my branch ('Implement
the Hostname property'), where I did not add the property to
'ChannelProperties'. I found it better to squash a fix for that in the same
commit (the other commits in the branch are untouched).

> This is a pointer to the GabbleConnection without a reference; it implicitly
> assumes that the last ref to the ChanneManager will be the one held by
> the GabbleConnection. I'd prefer this to be g_value_dup_object (introducing a
> cyclic reference), with the cyclic reference released when the
> GabbleConnection signals status-changed(DISCONNECTED).

Fixed in 80c02d9a756773ffafbccd8b2577c5cfc218ac59
I also made the manager release the reference in _dispose(), as
tp_clear_object() will take care of making the whole thing safe for us anyway.

> > +  if (self->priv->verify_async_called)
> > +    {
> > +      DEBUG ("verify_async() called multiple times, returning.");
> > +      return;
> > +    }
> 
> If it would be wrong for Wocky to call this more than once, then you want
> g_return_if_fail (!self->priv->verify_async_called), which expands to much
> the same thing, but with a g_critical instead of a g_debug.

Fixed in ed7c9ba0044484cc559ca3d55a9572f95e141d7a

> You should chain up to the parent's constructed() *before* running your own.
> 
> I prefer to use a temporary variable for the implementation, to avoid
> doing the G_OBJECT_CLASS type-check twice, as seen in
> gabble_connection_manager_constructed().
> 
> (Channel and Certificate have the same issues.)

Done in 8fce08d237805e9543f0a82e4fadf224c00394fc

> > +  /* there's only one channel of this kind */
> > +  g_object_get (self->priv->channel, "channel-destroyed", &closed, NULL);
> 
> Can closed ever be true in your new logic? I don't think it can?

You're right, it can't, as the reference to the channel is released immediately
when the channel is closed.
I removed that bit in e29a48aab5138ae0a80f90b06b7598aec10da6a1

> I think it'd be worth having:
> 
> #define EASY_CASE(x) \

Right, this simplifies the code somewhat. I did this in
d7182b2629d859d5d8d2573ff6617f6f1bf1dddd

> In the Channel:
> > +  LAST_PROPERTY
> 
> I don't like the naming convention where a number 1 higher than the last
> property is called LAST_PROPERTY :-P If you need it at all, it should be
> called N_PROPERTIES or something.

It's not actually needed, but it's a GObject style convention I always
followed, and I see other objects in Gabble do the same.
Anyway, I renamed it to NUM_PROPERTIES in
570505b2cc5403e7331ff6a1e2d0225e36728b6b

> I don't think you need these assertions. If dbus-glib is calling these
> methods on a non-GabbleServerTLSChannel then the GABBLE_SERVER_TLS_CHANNEL
> checked-cast will already have failed with g_critical() anyway, and we'll
> segfault by dereferencing NULL a moment later.

Good point; I removed it in d478168bb7224112281edf848b6ab234928866d4

> Doesn't GabbleBaseChannel implement GetInterfaces()? If it does, you don't
> need to override it in channel_iface_init() - it's OK to override *part*
> of a GInterface.

Yeah, GabbleBaseChannel already takes care of it; that function was a leftover
of when the channel was not a GabbleBaseChannel subclass, and I removed it in
c34638fbb2651dc054c9226dd32cc0b92611a6d0

> > +static void
> > +gabble_tls_certificate_finalize (GObject *object)
> ...
> > +  if (self->priv->reject_details != NULL)
> > +    g_hash_table_unref (self->priv->reject_details);
> 
> tp_clear_pointer (&self->priv->reject_details, g_hash_table_unref)?

Fixed in 1080b9a5fea51394f2c75d94c6c49cbf0987168d

> > +  pspec = g_param_spec_boxed ("reject-details",
> > +      "The reject error details",
> > +      "Additional information about the rejection of the certificate",
> > +      G_TYPE_HASH_TABLE,
> 
> If this is exported on D-Bus, dbus-glib won't understand it. I think you
> need TP_HASH_TYPE_STRING_VARIANT_MAP (which is a dbus-glib parameterized
> type).

Fixed in 6cae55d054075c41eab0514da4b2125cf004ceec

> > +  self->priv->reject_details = g_hash_table_new_full (g_str_hash, g_str_equal,
> > +      (GDestroyNotify) g_free, (GDestroyNotify) tp_g_value_slice_free);
> 
> g_free is already a GDestroyNotify, you shouldn't need to cast it.

Fixed in ff67002a487ee585d8293e2609980cf08071faf3

> > +  self->priv->server_tls_manager = g_object_new (GABBLE_TYPE_SERVER_TLS_MANAGER,
> > +      "connection", self, NULL);
> 
> Indentation: each property/value pair on a new line (even if, as in this case,
> there's only one pair)
> 
> > @@ -1606,6 +1637,7 @@ connector_error_disconnect (GabbleConnection *self,
> >  
> >    gabble_connection_disconnect_with_tp_error (self, tp_error, reason);
> >    g_error_free (tp_error);
> > +
> >  }
> 
> Spurious blank line

All fixed in 8d4a22a9e969974d53a79026319aff6c009fa122

> > +      if (details != NULL)
> > +        g_hash_table_unref (details);
> 
> tp_clear_pointer?

Fixed in 1080b9a5fea51394f2c75d94c6c49cbf0987168d

> > Bump telepathy-glib required version to 0.11.12
> 
> Why?

Because we use the new TP_CONNECTION_STATUS_REASON_CERT_* values of the
TpConnectionStatusReason enum.

> Ah, I see the problem. CertificateData isn't actually a "simple type" (as
> defined in my head, at least), it's a container (array of byte); we make
> type-generating functions for structs and dicts, and arrays of those (perhaps
> recursively), but not for arrays of (arrays of...) non-container types like
> byte.
> 
> To be honest, telepathy-glib should probably just grow a non-generated
> TP_ARRAY_TYPE_UCHAR_ARRAY_LIST.

Ok, I opened bug 29671 against telepathy-glib for it.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.