Bug 51033 - Add support for see-other-host
Add support for see-other-host
Status: RESOLVED FIXED
Product: Telepathy
Classification: Unclassified
Component: gabble
unspecified
Other All
: medium normal
Assigned To: Telepathy bugs list
Telepathy bugs list
http://cgit.collabora.com/git/user/xc...
: patch
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-13 05:45 UTC by Xavier Claessens
Modified: 2012-06-20 07:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Xavier Claessens 2012-06-13 05:45:23 UTC
It's part of XMPP CORE spec and it's now used by WLM server.

http://xmpp.org/rfcs/rfc6120.html#streams-error-conditions-see-other-host
Comment 1 Xavier Claessens 2012-06-13 05:47:46 UTC
Wocky branch:
http://cgit.collabora.com/git/user/xclaesse/wocky.git/log/?h=see-other-host

Gabble branch:
http://cgit.collabora.com/git/user/xclaesse/telepathy-gabble.git/log/?h=see-other-host

I don't know if that's the best way to implement this, I'm not wocky/gabble expert. But at least it works with the WLM server.

This is pretty critical, since this morning, I couldn't connect to WLM anymore without those patches.
Comment 2 Jonny Lamb 2012-06-14 04:53:00 UTC
This looks pretty much how I would do it too.

> + connect_to_host_async (self, other_host, 5222);

Why are you specifying 5222? The other host might have specified a different port, no?

A Wocky test would be pretty cool... :-)
Comment 3 Xavier Claessens 2012-06-14 05:07:05 UTC
(In reply to comment #2)
> This looks pretty much how I would do it too.

Cool :)

> > + connect_to_host_async (self, other_host, 5222);
> 
> Why are you specifying 5222? The other host might have specified a different
> port, no?

That ends into g_socket_client_connect_to_host(). As I understand it, port is a fallback in the case host does not contain the port. WLM server does not set a port with the host.

> A Wocky test would be pretty cool... :-)

Actually the gabble patch breaks one of its unit test, because CM should not close the channel on the behalves of the handler. I'll work on a fix.
Comment 4 Jonny Lamb 2012-06-19 07:19:50 UTC
+ p = g_strstr_len (host_and_port, -1, ":");
+ if (p != NULL)
+   p = g_strstr_len (p + 1, -1, ":");
+ if (p != NULL)
+   uri_format = "%s://[%s]";

It took me a little while to work out what you were actually checking for here. Could you just add a small comment to point out you're looking for two ':' chars please?

- /* properties: */
- GIOStream *stream;
-

Why are you removing this? It looks completely unrelated to the rest of the patch. If it's unused feel free to remove it, but please split your patches.

I don't exactly understand what you are doing in the test but tbh if things still test properly and stuff passes and your test case is being executed then that's fine by me. :-)
Comment 5 Xavier Claessens 2012-06-19 08:46:41 UTC
(In reply to comment #4)
> + p = g_strstr_len (host_and_port, -1, ":");
> + if (p != NULL)
> +   p = g_strstr_len (p + 1, -1, ":");
> + if (p != NULL)
> +   uri_format = "%s://[%s]";
> 
> It took me a little while to work out what you were actually checking for here.
> Could you just add a small comment to point out you're looking for two ':'
> chars please?

Added /* if host_and_port contains 2 ':' it must be an ipv6 */

> - /* properties: */
> - GIOStream *stream;
> -
> 
> Why are you removing this? It looks completely unrelated to the rest of the
> patch. If it's unused feel free to remove it, but please split your patches.

It's unused, pushed that trivia to master already :)

> I don't exactly understand what you are doing in the test but tbh if things
> still test properly and stuff passes and your test case is being executed then
> that's fine by me. :-)

It though me a while to understand how those unit tests works. The idea of my change is to be able to run multiple servers, so the first will do a see-other-host error and tell to connect the the next one.
Comment 6 Xavier Claessens 2012-06-19 08:51:42 UTC
(In reply to comment #3)
> Actually the gabble patch breaks one of its unit test, because CM should not
> close the channel on the behalves of the handler. I'll work on a fix.

My branch now fix that as well. It's much more complex than I first though, because it can now happen that we have more than one TLS channel at the same time. This is because it can happen that wocky start connecting to next host (after a see-other-host) before handler of the previous TLS check called Close(). Actually with empathy that always happen because empathy never close TLS channels.
Comment 7 Xavier Claessens 2012-06-19 09:06:11 UTC
wocky branch pushed, gabble branch still needs review.
Comment 8 Jonny Lamb 2012-06-19 09:18:19 UTC
Regarding your gabble branch:

Personally I think a nicer way than having both

     GabbleServerTLSChannel *channel
     GList *completed_channels;

in your private structure, you could just have a GQueue *channels and peek the head when you mean priv->channel, no? I guess that would make referencing the GAsyncResult with channel harder. What do you think?
Comment 9 Xavier Claessens 2012-06-19 09:25:07 UTC
(In reply to comment #8)
> Regarding your gabble branch:
> 
> Personally I think a nicer way than having both
> 
>      GabbleServerTLSChannel *channel
>      GList *completed_channels;
> 
> in your private structure, you could just have a GQueue *channels and peek the
> head when you mean priv->channel, no? I guess that would make referencing the
> GAsyncResult with channel harder. What do you think?

I think there are a lot of priv->channel access in that code, modifying them all to g_queue_peek_head() is boring :p

IMO it's nice to have all the current operations stuff in the priv struct, then have a list for stuff from the past.
Comment 10 Jonny Lamb 2012-06-20 05:26:30 UTC
(In reply to comment #9)
> I think there are a lot of priv->channel access in that code, modifying them
> all to g_queue_peek_head() is boring :p

Hardly...

> IMO it's nice to have all the current operations stuff in the priv struct, then
> have a list for stuff from the past.

Oh well, up to you then, there are probably much worse things in gabble.

Merge away.
Comment 11 Xavier Claessens 2012-06-20 07:29:34 UTC
Backported to stable branch, released as 0.16.1.