[Bug 23640] [regression] Re-implement proxy support

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2010-09-07 08:08:23 PDT ---
First thanks for this review. I've done the fixes in separate patches that I'll
sqash later, this way it's easier for you to recheck that I have not done any
mistakes.

(In reply to comment #4)
> I'm sad that we're parsing strings from a list, when we have a perfectly good
> structured type system, but I can see why you did this (we don't have a sane
> serialization for anything else in a keyfile) and I can't think of anything
> better we could do.
> 
> > +  guint fallback_server_idx;
> 
> ..._index - please avoid uncommon abbreviations.

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=9d75699f633a1388437717a5096ab6c697969b90

> 
> It would be nice to have a concrete example of what we would use for Google
> Talk, either in the code or in the commit message. I infer that it would be
> something like this:
> 
>     ["talk.l.google.com",
>     "talk.l.google.com,443,1"]
> 
> The commit message says the syntax has colons and the implementation says it
> has commas: please fix one or the other.

Improved commit message:
http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commit;h=d87799deb33c10940b7c99c6104d07ccd57280ed

> 
> I assume that the reason for using commas was that colons conflict with IPv6
> literals' use of colons, but using commas is weird and unconventional too.
> Possibilities:
> 
> * use commas anyway
> * use colons, support URI-style IPv6 literals like [::1]:5222 (hard,
> pointless?)
> * use colons, don't support IPv6 literals, assume that anyone with working IPv6
> will also have working DNS

I had to switch to comma because : clash with IPV6 has you mentioned and ;
clash with Mission Control serialization of 'as'. The main goal was to ensure
that a simple strplit would make the job.

> 
> Given its purpose, it's not at all clear to me that we'd ever want IP literals
> in the fallback server list at all, let alone IPv6 literals.
> 
> > +        old_ssl = strcmp (split[2],"1") == 0;
> 
> The part after " = " should have parentheses for clarity, or (better) be
> implemented in terms of tp_strdiff().
> 
> I don't much like this syntax for old-SSL anyway, though; if nothing else,
> ",oldssl" is more mnemonic than ",1".

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=497aabcccda38d397a17a1e3bcb330d599fcdc91

> 
> Are we ever likely to want to fall back to old-SSL on a port that is neither
> 5223 nor 443, or normal XMPP on port 5223 or 443? If not, we could perhaps
> decide whether to use old-SSL based on the port number?
> 
> (That question basically means: are we ever likely to use this for a service
> that isn't Google Talk, and if so, are we ever likely to use it for a service
> that is very strangely set up?)

In my opinion, the cost of being flexible is very low, so why not ?  Also, port
number is very weak heuristic for old-SSL. Like any server, you are free to run
it on the port you want. XMPP might be used as intranet communication, in which
case we may want to use different port for technical reason.

> 
> > +next_fallback_server (GabbleConnection *self, const GError *error)
> 
> Style: new line for the second argument please

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=f9b7e137c0c5b914fdcbc4a8bee455172e22e2e0

> 
> > + * FALSE is they all have been tried.
> 
> Typo: "FALSE if they"

http://git.collabora.co.uk/?p=user/nicolas/telepathy-gabble.git;a=commitdiff;h=84264b4c918ffff782ad8d864281d181d5969a7f

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