[Bug 29458] TLS-connection branch

[bugzilla-daemon at freedesktop.org]

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

--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-08-18 02:54:38 PDT ---
Thanks, this looks much better. I still have a few complaints:

In the ChannelManager:
> +    case PROP_CONNECTION:
> +      self->priv->connection = g_value_get_object (value);
> +      break;

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

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

> +static void
> +gabble_server_tls_manager_constructed (GObject *object)
> +{
> +  GabbleServerTLSManager *self = GABBLE_SERVER_TLS_MANAGER (object);
> +
> +  DEBUG ("Server TLS Manager constructed");
> +
> +  gabble_signal_connect_weak (self->priv->connection, "status-changed",
> +      G_CALLBACK (connection_status_changed_cb), object);
> +
> +  if (G_OBJECT_CLASS
> +      (gabble_server_tls_manager_parent_class)->constructed != NULL)
> +    {
> +      G_OBJECT_CLASS
> +        (gabble_server_tls_manager_parent_class)->constructed (object);
> +    }
> +}

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

> +  /* 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?

Telepathy style for g_object_get, and similar functions with pairs/triples
of arguments, looks like:

    g_object_get (self->priv->channel,
        "channel-destroyed", &closed,            /* one pair per line */
        NULL);

> +    case GABBLE_TLS_CERTIFICATE_REJECT_REASON_UNTRUSTED:
> +      retval = TP_CONNECTION_STATUS_REASON_CERT_UNTRUSTED;
> +      break;

I think it'd be worth having:

#define EASY_CASE(x) \
    case GABBLE_TLS_CERTIFICATE_REJECT_REASON_ ## x: \
        retval = TP_CONNECTION_STATUS_REASON_CERT_ ## x; \
        break;

    switch (tls_reason)
      {
        EASY_CASE (UNTRUSTED)
        EASY_CASE (EXPIRED)
        ...
        case blah_blah_UNKNOWN:
            retval = blah_blah_OTHER_ERROR;
            break;
      }

#undef EASY_CASE

Also note that you can return from inside a switch; in a simple function
like this one, you could replace each "retval =" with "return", delete all
the break statements (which would become unreachable), and delete the
return statement at the end and the variable retval.

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.

> +static void
> +gabble_server_tls_channel_impl_close (TpSvcChannel *iface,
> +    DBusGMethodInvocation *context)
> +{
> +  GabbleServerTLSChannel *self = GABBLE_SERVER_TLS_CHANNEL (iface);
> +
> +  g_assert (GABBLE_IS_SERVER_TLS_CHANNEL (self));

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.

It's particularly unnecessary in
gabble_server_tls_channel_impl_get_interfaces, where @self isn't actually
used.

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.

> +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)?

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

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

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

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

tp_clear_pointer?

> Bump telepathy-glib required version to 0.11.12

Why?

(In reply to comment #2)
> I fixed all of your points, except for this
> 
> > > +      /* FIXME: this should be generated for us. */
> > > +      array_of_ay_get_type (),
> > 
> > Don't you get GABBLE_ARRAY_TYPE_CERTIFICATE_DATA_LIST in
> > extensions/_gen/gtypes.h?
> 
> No, it doesn't get generated. Probably a bug in the tp-parser code with array
> tp:simple-types? Shall I file a bug for that?

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.

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