Bug 29458

In general I prefer the first review round for a branch to be a series of coherent patches (i.e. I prefer it if people use git rebase --interactive to squash trivial fixes into the patch that they fix, before the first review round; ideally, make check should pass and the code should make sense after each commit).

As a first review pass here I've mostly gone through the whole diff without looking at individual patches, so you're free to rebase where appropriate. There are changes in this branch that shouldn't be there, so please do rebase them out.

Irrelevant changes
==================

> --- a/src/ft-channel.h
> +++ b/src/ft-channel.h
> ...
> -#include <extensions/_gen/svc.h>
> -#include <extensions/_gen/interfaces.h>
> -#include <extensions/_gen/enums.h>
> +#include <extensions/extensions.h>

I'm sure this change is good and all, but it's not part of your feature. This sort of thing could usefully have been fast-tracked in as part of a small bugfix branch (Telepathy people tend to call that branch "trivia", which is probably my fault).

> -  g_assert ((priv->state == JS_STATE_PENDING_CREATED) ||
> -      (priv->state == JS_STATE_ENDED));
> +  g_assert ((priv->state == (JingleState) JS_STATE_PENDING_CREATED) ||
> +      (priv->state == (JingleState) JS_STATE_ENDED));

Likewise, and also, I don't like scattering casts through our code if we don't have to; they clutter the code (hurting clarity), and can mask genuine bugs that the compiler would have warned us about. I'll open a separate bug for the gcc 4.5 stuff.

I'm going to assume that all changes in ft-channel.h, jingle-session.c, muc-channel.c, media-factory.c, tubes-channel.c are irrelevant to this branch.

Architectural thoughts
======================

I'm unhappy about subclassing WockyTLSHandler in the Channel; it means you can't subclass GabbleBaseChannel, so you need a lot of boilerplate. The many-one and one-one relationships also don't seem right this way.

Is there a reason why the *channel manager* can't be the WockyTLSHandler and do most of the work? If you did that, I think the architecture would make more sense:

* it could avoid creating the Channel until verify_async() is called, meaning the Channel could put itself on the bus, create its certificate, etc. in constructed() rather than having a separate method to do that
* it could create a Channel every time verify_async() is called, if for some insane reason Wocky calls it multiple times

The channel manager should manage the channel(s), rather than being a thin shim around a channel that manages itself :-)

Minor details
=============

> @@ -1599,6 +1620,36 @@ connector_error_disconnect (GabbleConnection *self,
> +      channel = GABBLE_SERVER_TLS_CHANNEL
> +        (gabble_server_tls_manager_get_handler (self->priv->server_tls_manager));

Why does this need a cast?

Could the TLS manager do the heavy lifting here, to keep the messy stuff out of GabbleConnection? Something like this, perhaps:

TpConnectionStatusReason reason;
GHashTable *details;

if (gabble_server_tls_manager_get_rejection (self->priv->server_tls_manager,
        &dbus_error, &details, &reason))
  {
    tp_base_connection_disconnect_with_dbus_error (base,
      dbus_error, details, reason);
    g_free (dbus_error);
    g_hash_table_unref (details);
    return;
  }

> +++ b/src/error.c
> +typedef struct {
> +  GabbleTLSCertificateRejectReason tls_reason;
> +  TpConnectionStatusReason tp_reason;
> +} TLSToTpReasonMap;

I think this might be clearer as a switch() in the function gabble_tls_reason_to_conn_reason. A switch() is also O(1) instead of O(n), if you like micro-optimizations.

If the TLS manager does the heavy lifting as I suggested above, then this could all go in the TLS manager's source file, and the TLS manager's API to the Connection would just be to return the right error.

> +++ b/src/tls-certificate.h
> +#include <wocky/wocky-tls.h>

This doesn't seem to be necessary for the header, only for the implementation?

> +void
> +gabble_tls_certificate_register (GabbleTLSCertificate *self)
> +{
> +  DBusGConnection *bus;
> +
> +  /* register on the bus */
> +  bus = tp_get_bus ();

I'd prefer new code to take a TpDBusDaemon as a construct-only property (or obtain it from its TpBaseConnection/GabbleConnection, if it has one), and use tp_dbus_daemon_register_object() on it. (The same applies to the channel.)

Given that a GabbleTLSCertificate knows its own object path and immutable properties, why would it be necessary to have a delay between creation and export? You could just export it in the constructed() GObject pseudo-method.

> +  DEBUG ("Created TLS certificate, type %s, at path %s, with data %p",
> +      cert_type, object_path, cert_data);

This is in gabble_tls_certificate_new(), so it won't run if an object is created by calling g_object_new() directly (e.g. in a subclass). If this debug message is useful, the init(), constructor() or constructed() would be the place to put it. I realise you don't have any subclasses here, but having code other than g_object_new() in a _new() wrapper is a bad habit to get into.

> +/* public */
> +GabbleTLSCertificate *
> +gabble_tls_certificate_new (const gchar *object_path,

Why is it necessary to say this is public? The comment doesn't seem very useful.

> +gabble_tls_certificate_reject (GabbleSvcAuthenticationTLSCertificate *cert,
...
> +  g_assert (self->priv->cert_state == GABBLE_TLS_CERTIFICATE_STATE_PENDING);

Don't assert on D-Bus input (and call sequences are input). Calling one of Accept() or Reject() while in a state other than PENDING should send an appropriate error to the caller, not crash Gabble.

> +  retval = g_object_new (GABBLE_TYPE_TLS_CERTIFICATE,
> +      "object-path", object_path, "certificate-chain-data",
> +      cert_data, "certificate-type", cert_type, NULL);

Please break lines in logical places when varargs are paired or otherwise linked:

    g_object_new (TYPE,
        "first-prop", first_value,
        "second-prop", second_value,
        NULL);

(In Telepathy style, this even applies if the number of pairs is 0 or 1.)

> +    guint in_Reason,
> +    const gchar *in_Error,
> +    GHashTable *in_Details,

The codegen generates these names as a cheap way to get non-colliding parameter names that aren't C keywords. Actual code written by actual people should use conventional parameter names (I'd use reason, error and details here).

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

> +static void gabble_server_tls_channel_verify_async (WockyTLSHandler *handler,
> +    WockyTLSSession *tls_session, const gchar *peername,
> +    GAsyncReadyCallback callback, gpointer user_data);

Why's this forward-declared? I try to avoid static forward declarations unless they're really needed. (channel_iface_init is really needed, because it's used in G_DEFINE_TYPE_WITH_CODE.)

> +static const gchar *gabble_server_tls_channel_interfaces[] = {
> +  GABBLE_IFACE_CHANNEL_TYPE_SERVER_TLS_CONNECTION,
> +  NULL
> +};

The channel type doesn't need to appear in Interfaces.

> +gabble_server_tls_channel_init (GabbleServerTLSChannel *self)
> +  self->priv->closed = TRUE;

This business with setting closed initially, and clearing it on export, seems rather astonishing. What's your intention here? Why is starting with closed=FALSE insufficient?

> +  if (self->priv->server_cert != NULL)
> +    g_object_unref (self->priv->server_cert);

tp_clear_object (&self->priv->server_cert) is less code :-)

Releasing references in finalize isn't right; they should be released in dispose. Only things that don't hold a reference should be released in finalize.

> +GabbleTLSCertificate *
> +gabble_server_tls_channel_get_certificate (GabbleServerTLSChannel *self)
> +{
> +  if (self->priv->tls_state_changed)
> +    return self->priv->server_cert;
> +  else
> +    return NULL;
> +}

What's going on here? The server cert exists, regardless...

> +static void
> +tls_certificate_rejected_cb (GabbleSvcAuthenticationTLSCertificate *cert,
> +    guint reason,

Shouldn't this be a member of WockyTlsCertError?

> +  g_object_unref (self->priv->async_result);
> +  self->priv->async_result = NULL;

tp_clear_object() is my new favourite hammer :-P

> +cert_type_to_str (WockyTLSCertType type)

This could usefully use a switch().

Isn't "X509" meant to be "x509" per telepathy-spec? Either change the implementation or the spec (and I think the CertificateType needs to be machine-readable rather than human-readable, so the spec's right and the implementation's wrong).

Similarly, isn't "OpenPGP" meant to be "pgp"?

Will WOCKY_TLS_CERT_TYPE_NONE ever appear here? It probably shouldn't...

(In reply to comment #1)
> In general I prefer the first review round for a branch to be a series of
> coherent patches (i.e. I prefer it if people use git rebase --interactive to
> squash trivial fixes into the patch that they fix, before the first review
> round; ideally, make check should pass and the code should make sense after
> each commit).
> 
> As a first review pass here I've mostly gone through the whole diff without
> looking at individual patches, so you're free to rebase where appropriate.
> There are changes in this branch that shouldn't be there, so please do rebase
> them out.

Ok, I will try to follow this workflow in the future.
In the meanwhile, I squashed and rebased my branch in logical pieces now, so it's probably easier to follow. The URL is the same [1].

> > -  g_assert ((priv->state == JS_STATE_PENDING_CREATED) ||
> > -      (priv->state == JS_STATE_ENDED));
> > +  g_assert ((priv->state == (JingleState) JS_STATE_PENDING_CREATED) ||
> > +      (priv->state == (JingleState) JS_STATE_ENDED));
> 
> Likewise, and also, I don't like scattering casts through our code if we don't
> have to; they clutter the code (hurting clarity), and can mask genuine bugs
> that the compiler would have warned us about. I'll open a separate bug for the
> gcc 4.5 stuff.

Ok; I needed this for actual compilation, so it's just the first solution that made code build. Anyway, I left it out of the branch, you can find the same patch here for reference [2].

> Architectural thoughts
> ======================
> 
> I'm unhappy about subclassing WockyTLSHandler in the Channel; it means you
> can't subclass GabbleBaseChannel, so you need a lot of boilerplate. The
> many-one and one-one relationships also don't seem right this way.
> 
> Is there a reason why the *channel manager* can't be the WockyTLSHandler and do
> most of the work? If you did that, I think the architecture would make more
> sense:
> 
> * it could avoid creating the Channel until verify_async() is called, meaning
> the Channel could put itself on the bus, create its certificate, etc. in
> constructed() rather than having a separate method to do that
> * it could create a Channel every time verify_async() is called, if for some
> insane reason Wocky calls it multiple times

Great idea, I didn't think about this approach; the code indeed looks nicer and cleaner now. I fixed my branch to follow this approach, except for your last point (calling verify_async() multiple times from wocky), which should never happen (so I special-cased that in my _verify_async() implementation).

> Minor details
> =============

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?

[1] http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=shortlog;h=refs/heads/tls-connection2
[2] http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=shortlog;h=refs/heads/gcc4.5

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.

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

(In reply to comment #4)
> > > 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.

OK, good. In future I'd prefer it if the commit message said why, but there's no need to change this now.

Your changes look good to me, r+.

Pushed to master, thanks for the review.