[Telepathy] review: Gabble hash caps

[Alban Crequy]

Branch:
http://monkey.collabora.co.uk/telepathy-gabble-alban-xep0115/

XEP-0115 v1.3 (Entity Capabilities) was already implemented in Gabble.
This branch implements XEP-0115 v1.5 (the new version) and remains
compatible with Jabber client using the old version. The new version
hopefully uses less bandwidth for the caps and helps prevent poisoning
of caps.

Thanks for the review. Comments below:


Le Tue, 27 May 2008 20:18:05 +0100,
Dafydd Harries <dafydd.harries at collabora.co.uk> a écrit :

> 
> First pass:
> 
> +  str = g_string_free (s, FALSE);
> +  DEBUG ("caps string: '%s'", str);
> +  sha1_bin (str, strlen (str), (guchar *) sha1);
> +  encoded = base64_encode (SHA1_HASH_SIZE, sha1, FALSE);
> +  DEBUG ("caps base64: '%s'", encoded);
> 
> Don't you leak str here? I think we can call sha1_bin (s->str,
> s->len, ...), and then g_string_free (s, TRUE).

Right. Fixed.

> +          if (! g_str_equal (xmlns, "jabber:x:data"))
> +            continue;
> +
> +          if (! g_str_equal (type, "result"))
> +            continue;
> 
> These can crash if xmlns or type is NULL. Use g_strdiff instead.

Fixed.

> -  uris = _extract_cap_bundles (lm_node);
> +  uris = _extract_cap_bundles (lm_node, &hash, &ver);
> +  DEBUG ("_extract_cap_bundles get hash='%s' ver='%s'", hash, ver);
> 
> Perhaps _extract_cap_bundles should be renamed to something like
> _parse_caps_node?

Done.

> +  /* Only 'sha-1' is mandatory to implement by XEP-0115. If the
> received
> +   * discovery response uses another hash algorithm, don't check the
> hash and
> +   * fallback to the old method. */
> +  if (!tp_strdiff (waiter_self->hash, "sha-1"))
> 
> The comment doesn't seem to match the code here. It seems to be
> comparing against the hash algorithm indicated in the original caps
> node, not one indicated in the disco reply. Can you clarify?

It compares against the hash algorithm indicated in the presence stanza
that causes the discovery request and response.

I fixed the comment.

> +  waiter_self = NULL;
> +  for (i = waiters; NULL != i;  i = i->next)
> 
> Perhaps we should be using a hash table here.

Maybe.
(I did not change the code)

> -        self.remote_caps = { 'ext': 'voice-v1 jingle-audio
> jingle-video', 'ver': '0.6.0',
> -                 'node': 'http://telepathy.freedesktop.org/caps' }
> +        self.remote_caps = { 'ext': '', 'ver': '0.0.0',
> +                 'node': 'http://example.com/fake-client0' }
> 
> Hmm, was this causing some problem?

In previous versions of Gabble, Gabble has a bundle cache pre-filled
with "jingle-audio", etc. Now with this branch, Gabble does not has this
cache pre-filled, so it has to ask the caps of the remote contact
(the remote contact is simulated by the twisted test). It involves
some additional round trips to ask the caps of each bundle. But this
test is not about caps but about jingle, so in order to keep Jingle
tests simple, I don't use bundles and advertise all features directly.

The alternative would be to add some code in Jingle tests to expect
Gabble requesting each cap bundles, and reply to theses requests.

Unrelated note: the remote contact in this test suite use XEP-0115 v1.3.

> +    _test_without_hash(q, bus, conn, stream, 'bob1 at foo.com/Foo', 2L,
> client, 1)
> 
> s/1/True/.

Fixed

> +caps_changed_flag = 0
> 
> s/0/False/.

Fixed

> +def dbus_sync(bus, q, conn):
> +    # Dummy D-Bus method call
> +    call_async(q, conn, "InspectHandles", 1, [])
> +
> +    event = q.expect('dbus-return', method='InspectHandles')
> 
> I think I've written this before, but using org.freedesktop.Ping.

I don't see any test using org.freedesktop.Ping. Does Gabble really
implement the interface org.freedesktop.Ping? I don't see it in d-feet.

I use dbus_sync() to be sure Gabble finished to send its D-Bus signals
and that the twisted suite received all of them. So I need
Ping/InspectHandles/whatever to be implemented by Gabble (I cannot
just ping the dbus-daemon).

> Perhaps worth moving into servicetest.py.

Yes, done. It still uses 'InspectHandles' though.

-- 
Alban