On Sat, May 1, 2010 at 8:43 PM, Dario Freddi <drf54...@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? Account::create*DBusTube: please comment them out as long as they aren't actually implemented ;) (That is, if a merge is planned at this point). Also, I think "SingleDBusTube" could be clearer as "P2PDBusTube", as that's the terminology we've otherwise used. TubeChannel: I think it indeed makes sense to not have a separate TubeState feature (which you've commented out already), as any user of a tube channel will be interested in the state. As a side note, the warning output in tubeState() actually says FeatureChatState must be requested ;) 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. 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? As for StreamTubeChannel, the separate FeatureMonitorConnections makes sense - although I'd call it ConnectionMonitoring, to keep with the noun naming of most other Features. 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). 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. Also, considering newRemote/newLocal is only ever emitted for outgoing / incoming tubes respectively, maybe they should be moved to the directional subclasses? One more thing is you should consider building up Contacts for the NewRemoteConnection signal like Channel does for the group membership changes - it shouldn't be too hard here, but will allow applications to live with one less asynchronous call step. 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). Also, the offer methods seem to return NotCapable if the CM doesn't support the requested socket type / access control combo. NotCapable actually means "the *contact in question* doesn't have sufficient capabilities for the operation to succeed" - NotImplemented would be more appropriate here. Also, the remote "not IPv4/6" case should be InvalidArgument. Maybe go over your error usage in the other classes as well. IncomingStreamTube: I think PendingIODevice needs to be changed to PendingStreamTubeConnection or somesuch with the option of also getting the Address out of it in addition to the IO device to cater for applications NOT using a qt iodevice for their network I/O, at least not very directly - I imagine these are fairly common when eg. a protocol library with a non-qt API is used. For example, krdc with telepathy support passes around KUrl instances that the tubes support has constructed from the address the tube Accept returns, and eventually it passes this to libvncclient using a bare C API. Similarly, the device() on IncomingStreamTube should be complemented with an address() as well. Maybe setAllowedAddress actually removes the need for the access control parameter? We could have having called setAllowedAddress successfully mean that the application wants Port access control, otherwise it's Localhost. We'd need a unsetAllowedAddress() and proper documentation though, but seems like a good idea at first otherwise though. Similar abuse of the NotCapable error occurs in IncomingStreamTube too. The examples seem ok from a quick look. --- Br, Olli Salli _______________________________________________ telepathy mailing list telepathy@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/telepathy