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 <[email protected]> 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 <[email protected]> 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ telepathy mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/telepathy
