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

[Dario Freddi]

Hello everyone,

(I'm attaching the patchset here as I don't have the possibility to push
stuff to my repository right now)

I addressed most of your concerns and implemented most of your suggestions.
Point to point:

2010/5/12 Olli Salli <ollisal at gmail.com>

> 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).
>

Ok, now I clearly see your point. Although, in this case it is not
applicable, as if we want to keep track of the connections, the signals must
be connected as soon as the feature is enabled. However, I implemented
connectNotify() in all of the new classes to spit out some warnings.


>
> >
> >>
> >> 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.
>

Ok - atm I keep track of all the connectionIds, we can remove the public
exposures before the merge.


>
> >>
> >> 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.
>

It's a very good idea and I implemented it with some changes: I did not do
two separate Pending* classes, but I created a single one returning the
address type and both addresses (just like I did in StreamTubeChannel).
Also, I named them {offer,accept}TubeAs{Tcp,Unix}Socket(), which seems to me
more clear and consistent.


>
> 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.
>

Done :)


>
> 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.
>

Done as well, with slightly different names and a caveat: supportsTCPSockets
is not really applicable here: I used supportedTcpProtocols, returning which
IP protocol version is supported.


>
> >
> > 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.
>

Well, before reading your mail, I fixed everything you raised in the
previous mails and amended like hell :\ so the commit scheme is very clear,
but it is to be rebased on the cmake branch, and my previous patches are to
be deleted. The last fixes went in as separate commits, after I read this :D

Thanks, and waiting for the usual feedback :)


>
> > -------------------
> >
> > Dario Freddi
> > KDE Developer
> > GPG Key Signature: 511A9A3B
> >
>
>
> --
>
> Br,
> Olli Salli
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/telepathy/attachments/20100514/ae8205f6/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tp-qt4-stream-tube-patchset.tar.bz2
Type: application/x-bzip2
Size: 20363 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/telepathy/attachments/20100514/ae8205f6/attachment-0001.bin>