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

[Olli Salli]

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.

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.

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> However, acceptTubeAsTcp/UnixSocket is fine
and logical.

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. 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*()!

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

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.

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.

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.

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.

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.

-- 

Br,
Olli Salli