[Telepathy] API Draft for high level tubes in tp-qt4

Dario Freddi drf54321 at gmail.com
Tue Jun 1 05:47:50 PDT 2010


Hi,

On Monday 31 May 2010 20:39:21 Olli Salli wrote:
> On Fri, May 14, 2010 at 7:52 PM, Dario Freddi <drf54321 at gmail.com> wrote:
> > Hello everyone,
> > 
> > I addressed most of your concerns and implemented most of your
> > suggestions.
> > 
> > Thanks, and waiting for the usual feedback :)
> 
> Okay, coming back for probably the last round of reviewing this
> stuff... Again, sorry for the immense delay.

No problem :) Let's go

> 
> TubeChannel::parameters(): Note that currently this is only ever
> populated for incoming tubes. Parameters for outgoing tubes are set in
> offer(), not on channel construction, so introspecting them after
> creation and not updating afterwards won't do. The solution would be
> to implement a mechanism (protected function in TubeChannel most
> likely?) to update the parameters after Offer() has successfully
> returned.

Correct - I'll have a look if I can implement it into private classes through 
inheritance or just declare it as protected.

> 
> OutgoingStreamTubeChannel: <nitpick> I think
> offerTubeAs{Tcp,Unix}Socket is slightly awkward naming. You are not
> offering a tube as a socket, be it tcp or unix, instead, you're
> offering said socket as a tube ;) Given that "tube" is in the class
> name already, I think the most intuitive naming would be
> offer{Tcp,Unix}Socket - that says you're offering a tcp/unix socket
> using the tube.</nitpick>

Yes, you're right :)

> However, acceptTubeAsTcp/UnixSocket is fine
> and logical.

Ok.

> 
> StreamTubeChannel::support*(): They assume Localhost is supported if
> ANY address control scheme is supported for a given protocol; while
> this might usually be true, the spec guarantees it only for IPv4.

Ok - I misread the spec.

> So
> for the other two, please do check for Localhost being present like
> you're currently doing for Port and Credentials. Also, I think you
> should use these methods in the offer() methods instead of
> reimplementing the checks for consistency, it's currently actually
> implemented correctly (checking for Localhost) in the offer() methods
> although it's not in support*()!

Roger that :)

> 
> OutgoingStreamTubeChannel::offer / PendingStreamTubeConnection (unix
> socket variant): While the D-Bus API has QByteArray for the unix
> socket addresses, as QLocalSocket and QLocalServer both use QString I
> think that's what we should expose to the user too - preventing them
> from mixing the various charset interpretation choices you can do the
> conversion with.

It sounds sensible

> One thing is, you should probably use Latin1
> conversions everywhere (as you're doing in eg. onTubeStateChanged)
> instead of UTF-8.

Ok

> 
> And yeah, do comment out connections(), or implement it fully - the
> only thing you need to add is to declare a struct
> StreamTube::Connection with publics id, contact and
> sourceAddress/credentialByte with the last two carrying around data of
> the used access control to error out in the accessor if it's not
> appropriate. Just exposing the connection id is useless, as you can't
> get the useful information related to an id in any way.

I think I'll delay this after everything has been merged, as it would require 
more API additions and I think it would be better to just have the basic stuff 
rolling. So I'll just comment it out and look for implementing it _after_ the 
merge.

> 
> However, I see a bigger problem with the connection tracking - you use
> lookupContactByHandle(). That function can freely return you NULL if
> the contact in question happens to be not cached - only by doing the
> asynchronous contactsForHandles() call can you always build it. You
> should do something similar to how Channel handles the Group signals -
> put a suitable context of the signal to a FIFO queue (needs to be
> shared between the new*Connection and connectionClosed handling), then
> always build the contact using contactsForHandles() for the head of
> the queue, after that PendingContacts finishes move to the next queue
> item etc. What you shouldn't do is allow the contact building to
> overlap, this way you could eg. reorder a connection getting removed
> and appearing in the wrong order.

It makes sense, I will do it that way.

> 
> If this contact building in a FIFO queue stuff seems a bit too much,
> feel free to just skip it but in that case remove / comment out ALL of
> the connection tracking - there's not much use for signals which might
> contain the contact (the useful bit) or NULL arbitrarily.

The idea is to implement what you said above and not expose the connection 
tracking - so that most of the internals would already be ready, and 
implementing it after the merge would be trivial - either for me or anyone 
else.

> 
> Please update the documentation accordingly with the changes, eg.
> there are still mentions of setAllowedAddress etc. and the accept() /
> offer() variants should probably refer to their corresponding
> support() methods, etc. Probably some more of these occurrences of
> docs lagging behind the actual impl lurking around.

Sure

> 
> Seems like that'd be it for this round. This is a fairly extensive and
> invasive chunk of work to design/review - thankfully, once we have
> this stuff settled here for good, the DBus tube stuff should be much
> more clearer conceptually.

Definitely :) thanks for the thorough review.

P.S.: Will send a new alert when all of those issues have been addressed and I 
will commit new fixes as new commits.

-- 
-------------------

Dario Freddi
KDE Developer
GPG Key Signature: 511A9A3B
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/telepathy/attachments/20100601/e18b95a5/attachment.pgp>


More information about the telepathy mailing list