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: <div class="gmail_quote">2010/5/12 Olli Salli <<a href="mailto:ollisal@gmail.com">ollisal@gmail.com</a>> <blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div><div></div><div class="h5">On Tue, May 11, 2010 at 8:20 PM, Dario Freddi <<a href="mailto:drf54321@gmail.com">drf54321@gmail.com</a>> 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 <<a href="mailto:drf54321@gmail.com">drf54321@gmail.com</a>> 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. > </div></div>Let's do that. We should do a general spec vs lib implementation audit sometime soon anyway. <div class="im"> >> 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)? > </div>Exactly. <div class="im"> >> >> 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. </div>Yeah, I mostly brought this up here as a general point in tp-qt4 subclassability. <div class="im"> > >> 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. </div>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). </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div class="im"> > >> >> 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? </div>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. </blockquote><div> Ok - atm I keep track of all the connectionIds, we can remove the public exposures before the merge. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div class="im"> >> >> 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 </div>Yeah, that's it. <div class="im"> >> >> 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. > </div>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. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> 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. </blockquote><div> Done :) </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> 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. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div class="im"> > > P.S.: About that, would you prefer a particular commit scheme for the final > patches? > </div>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. </blockquote><div> 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 :) </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div class="im"> > ------------------- > > Dario Freddi > KDE Developer > GPG Key Signature: 511A9A3B > </div>-- Br, Olli Salli </blockquote></div> <div style="visibility: hidden; display: inline;" id="avg_ls_inline_popup"></div><style type="text/css">#avg_ls_inline_popup { position:absolute; z-index:9999; padding: 0px 0px; margin-left: 0px; margin-top: 0px; width: 240px; overflow: hidden; word-wrap: break-word; color: black; font-size: 10px; text-align: left; line-height: 13px;}</style>