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

[Olli Salli]

On Tue, May 11, 2010 at 8:20 PM, Dario Freddi <drf54321 at gmail.com> wrote:
> Hi Olli, first of all thanks for the thorough review.
>
> On Tuesday 11 May 2010 18:36:02 Olli Salli wrote:
>> On Sat, May 1, 2010 at 8:43 PM, Dario Freddi <drf54321 at gmail.com> wrote:
>> > On Saturday 01 May 2010 16:19:10 Dario Freddi wrote:
>> >> Hello,
>> >>
>> >> I managed to implement a preliminary version of StreamTubes. There's
>> >> still a critical problem in the accept phase I cannot get over and I'd
>> >> like you to take a look as well. But one thing at a time.
>> >
>> > Quick update: I switched using Unix sockets in IncomingTubeChannel while
>> > the problem gets settled, and everything is working nicely :) I made the
>> > sockets exchange some random text messages and printing them to stdout.
>> > Also, to demonstrate that everything is working, the sender is using a
>> > standard QTcpServer with default listening arguments, while the receiver
>> > is using, as already said, a unix socket.
>>
>> Finally had time to take a more thorough look at your code. Some
>> observations:
>>
>> Account::createStreamTube: about including TargetHandle == 0 in the
>> request when contact is NULL: the spec states about TargetHandle that
>> "If this is present in a channel request, it must be nonzero". So, I
>> think you should return a PendingChannelRequest which fails
>> immediately if the contact isn't valid. However, I realize what you
>> did is what the other Account methods are doing as well - Andre, can
>> you give me a heads up on why this is so? Should we just change them
>> all in fact?
>
> I agree with you they should all be changed - although, given that it should
> be a global change, I think it would be better to have my stuff merged first
> with this approach, and then change everything with a single commit - or
> otherwise have the other methods changed before the merge.
>

Let's do that. We should do a general spec vs lib implementation audit
sometime soon anyway.

>> In fact, to have the separate feature would be in a sense
>> similar to having Connection's Status as an optional introspectable,
>> which is clearly wrong.
>
> Ok - so I assume it is right to have this bundled into
> TubeChannel::FeatureCore (which will be renamed to FeatureTube as Simon
> suggested)?
>

Exactly.

>>
>> The documentation for FeatureCore says it's implicitly added to the
>> set of features for isReady() and becomeReady() - is this really true?
>> I see the FT channel docs say the same, but I don't think such a
>> mechanism actually exists (while it would be useful). However,
>> becomeReady does have a default parameter of Channel::FeatureCore, but
>> that doesn't extend to subclasses. Andre? Maybe we could make
>> ReadinessHelper always assume all registered introspectables with
>> critical == true as requested, or something?
>
> I leave the ball to Andre here.

Yeah, I mostly brought this up here as a general point in tp-qt4
subclassability.

>
>> Not many applications are likely
>> to care about the signals, and we should prevent the unneeded wakeups
>> in those cases. However, one thing I'd like to be explored here is
>> implementing QObject::connectNotify to make the connection as-needed,
>> which would lead to both less chances of getting it wrong (expecting
>> the signals while the feature isn't requested) and possibly better
>> performance (per-signal granularity instead of connecting all three).
>
> Well, connectNotify just lets the class know when a connection has been
> estabilished. It could be useful for spitting out warnings in case you
> connected to a signal which is not supposed to be emitted as the corresponding
> feature has not yet been enabled, but its purpose is barely limited to letting
> you know when $something connected to $signal, and act accordingly.
>
> I agree it should be implemented to spit out warnings, though.

Well, the QObject docs say about connectNotify(): "it might be useful
when you need to perform expensive initialization only if something is
connected to a signal." - which is exactly what we'd do here, the
expensive initialization being doing a round-trip match rule add round
trip to the bus daemon and then being woken up every time that signal
pops up on the bus. In fact, this is exactly what QDBus internally
does when one connects to a signal in a QDBusAbstractInterface-derived
class (which our low-level proxy classes are). We're sort of throwing
that away by always connecting our high-level object relay signals to
the low-level signals at initialization time. Note that one needs to
do refcounting in disconnectNotify too, and maybe some interaction
with destroyed() (not sure if disconnectNotify would be called in that
case too).

>
>>
>> If we still go down the Feature route we might as well offer a
>> connections() method, with the StreamTubeChannel object internally
>> keeping track of the changes, keeping the signals for change
>> notification of that set.
>
> Ok - do you want to add a separate FeatureConnections for that, I suppose? I
> also suppose the connections method should eventually return a list of
> Contacts?

If there's a feature causing the D-Bus signals to be connected, that
same feature should also cause the tracking to happen - any memory
cost from keeping a list of small structs is negligible compared to
the wakeup penalty from the signals.

And yeah, the connection descriptors it returns should include Contact
objects rather than handles.

But I guess we can postpone the whole connection tracking stuff for
now. It can be implemented in a backwards compatible fashion later on.
Well, then the change notification for a set of connection structs
would be signals essentially having the struct contents unpacked (conn
id, contact, param) instead of a single parameter with the struct, but
I think that's convenient anyway. Also consistent with eg.
AccountManager's accountCreated/Removed signals having the account
object path (essentially an unique ID) as the parameter instead of a
AccountPtr.

>>
>> OutgoingStreamTube: You should move PendingOpenTube out from the
>> public header. That is, unless you're planning to add some useful API
>> to it, maybe access to the Offer params, in which case you should
>> change the offer() methods to admit they return PendingOpenTube *
>> (also requires you to make them have the PendingFailure functionality
>> by themselves).
>
> Yes, I wanted to do it but somehow forgot it, sorry for that. Although, since
> it needs to be processed by moc, I'd need a separate private header. What is
> the naming convention for that? I saw there's a stream-media-channel-
> internal.h, so I suppose it's <name>-internal.h

Yeah, that's it.

>>
>> Maybe setAllowedAddress actually removes the need for the access
>> control parameter?
>
> Maybe. There is still the Credential case which should be handled, even if
> there could probably be a similar way for doing it. Maybe if I had some
> enlightenments on how the Credential control works (as I don't really get what
> it stands for at the moment) I could come up with something.
>
> Given the above is solved, I'm all for removing the parameter.
>
> Is such a thing applicable for Outgoing tubes as well? If so, those functions
> might be moved down to TubeChannel and simpify the whole deal, maybe even by
> having private classes inheriting one from the other thanks to the power of
> Q_DECLARE_PRIVATE.
>

Oh sorry, I did indeed forget about Credentials. Well, that
authentication scheme is only applicable to unix sockets - for which
Port access control obviously doesn't make sense. Credentials means
sending a SCM_CREDENTIALS ancillary message when connecting to the
socket, which the kernel checks and which guarantees to the listening
process that the process making the connection is really from the same
local user.

One idea that springs to mind is to actually have two accept() methods
- acceptLocal(bool requireCredentials) -> PendingLocalSocket and
acceptTCP(const QHostAddress &allowedAddress = QHostAddress::Any, int
port = 0) -> PendingTCPSocket. The latter should probably be two
overloads,  The Pending* classes would provide you with
QLocalSocket/QTcpSocket *socket() and QByteArray / QHostAddress+int
address(). This way you could dispense with both the address family
selection "guess" and setting the parameter beforehand. Again, using a
third-party library not accepting a QIODevice might require a specific
address family to connect to, we can't necessarily just always force a
specific type of socket, be it tcp or unix, down their throats. Yeah,
unless there's something very wrong in my thinking here (entirely
possible since this is like the 100th idea on this subject), this is
exactly what we should do.

For outgoing tubes, Credentials authentication is applicable but Port
isn't (you can't limit the CM to make all its connections to your
listening service from just one port, as any connections besides the
first one would fail with EADDRINUSE). So, as we already have address
family -specific overloads for Offer, we should just drop the access
control param from the TCP variants since for those Localhost will be
the only available choice anyway. For the unix variants we should
change it to bool requiresCredentials, switching between using
Localhost and Credentials. Tada, away with the abstract access control
param.

This makes me think of another detail we could make more high-level:
how would changing StreamTubeChannel::supportedSocketTypes() to bool
supportsTCPSockets(), bool supportsTCPPortSelection(), bool
supportsLocalSockets() & bool supportsLocalCredentialPassing() with
appropriate documentation sound? Then it would be more obvious which
of the offer/accept variants are expected to actually work.

>
> P.S.: About that, would you prefer a particular commit scheme for the final
> patches?
>

Anything goes, but you should preserve as much of your original change
history as you dare. At least, don't overwrite commits from before a
review, instead fix them in later commits - this makes it easier to
spot the changes resulting from the review, and verify they fix the
issues as intended.

> -------------------
>
> Dario Freddi
> KDE Developer
> GPG Key Signature: 511A9A3B
>


-- 

Br,
Olli Salli