[Bug 29458] TLS-connection branch

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Cosimo Cecchi <cosimoc at gnome.org> 2010-08-10 11:00:07 PDT ---
(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

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