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

[Dario Freddi]

Update: please ignore the patches I attached previously (they were also 
flawed). Instead, pull again from my branch (warning, I -f push'd).

On Wednesday 12 May 2010 17:15:32 Olli Salli wrote:
> 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

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

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/20100517/16f62a37/attachment.pgp>