Bug 35084

An initial draft of the Offerer side API is at 

http://cgit.collabora.co.uk/git/user/oggis/telepathy-qt4.git/tree/TelepathyQt4/stream-tube-server.h?h=streamtube-handler&id=2cc2d465ce2b0d48f6bf6ab1ba07a4b1346db76d

Comments welcome.

The basic idea is that minimally, you only have to create the StreamTubeServer and call exportTcpSocket with your listen socket, and you'll get all outgoing tubes handled by that socket being Offered on the tube. A QTcpServer overload is provided for convenience, but it's only meant to extract the current listen address of the server, not establish any kind of linkup.

For richer Telepathy integration monitorConnections = true can be passed to the create() method. This allows associating particular connections to the listen socket with the Telepathy contact who connected to your exported service through a tube, and getting informed about the Telepathy-level connection errors (so you can get more than just a close event / read error on your socket). I imagine these would be useful for especially GUI applications, which might then e.g. show the face of whoever connected, but also for logging Telepathy metadata for connections in server daemons.

I had the UNIX socket support API drafted as well but I felt the TCP/UDP duplication took attention away from the overall concepts of the API. Perhaps it should be a separate class, if we need it at all? Are people in practice likely even want to export UNIX socket services? X11 and CUPS come to mind as potential uses, but both can be configured to also listen for localhost TCP connections.

Updated draft:

http://cgit.collabora.co.uk/git/user/oggis/telepathy-qt4.git/tree/TelepathyQt4/stream-tube-server.h?h=streamtube-handler&id=c70a05fb8791a24dce27d1ee4fe390c4316254c6

Branch updated.

It now has my current views on how both StreamTubeServer and StreamTubeClient APIs should look like, fully implements a common SimpleStreamTubeHandler part for them, and ports the tube-initiator example originally written by Dario to use the already-implemented parts of StreamTubeServer.

It's still missing connection tracking in both the Server and the Client, and actually accepting the tubes in the Client, porting the tube-receiver example to use Client and cleaning up the examples. ContactCapabilities should be used to pick up the remote contact firing up their receiver in tube-initiator, for example.

I don't have time to work on this anymore very soon, so I'm willing to review the work of anybody adopting it.

I'm re-taking this due to my other project finishing.

Initial review for current branch state. More to come later.

+    friend class StreamTubeServer; // interim until STS is implemented using tube wrapping
     TELEPATHY_QT4_NO_EXPORT SharedPtr<RefCounted> _object() const; // TODO: turn this into _object()
Is this still needed?

++++++ simple-stream-tube-handler

+        foreach (StreamTubeChannelPtr tube, mTubes.keys()) {
const StreamTubeChannelPtr &, not a ConstPtr

+        Q_ASSERT(!p2pServices.isEmpty() || !roomServices.isEmpty());
I don't think we should ASSERT here. Just fail ::create and warn if they are both empty.
This is internal API, but it's called directly from STubeClient/Server constructors with user provided params.

+        foreach (QString service, p2pServices)
const QString &

+        foreach (QString service, roomServices)
same here

+            const QString channelType =
+                chan->immutableProperties()[QLatin1String(
+                        TELEPATHY_INTERFACE_CHANNEL ".ChannelType")].toString();
This reminds me we should parse immutable properties on construction and properly return them on accessors when available. So you could use chan->channelType() directly here, even if the channel is not ready.

+void SimpleStreamTubeHandler::onReadyOpFinished(Tp::PendingOperation *op)
+{
+    Q_ASSERT(!mInvocations.isEmpty());
+    Q_ASSERT(!op || op->isFinished());
Well I guess this will break when called from here:
+        invocation->message = QLatin1String("Got no suitable channels");
+        onReadyOpFinished(0);
Also here you should properly check that context->setFinished is being called when
onReadyOpFinished(0) is called.
You could simplify this by just calling ctx->setFinished/WithError directly when no tube channel was provided in handleChannels()

+        foreach (StreamTubeChannelPtr tube, invocation->tubes) {
+            if (!tube->isValid()) {
const StremTubeChannelPtr &, not a ConstPtr
Please check other loops also for similar "issues".

Also it seems InvocationData leaks here.

Even SimpleStubeHandler being internal I would like to have header guards there.


++++++ stream-tube-client

+    foreach (TubeWrapper *wrapper, mPriv->tubes.values()) {
+        wrapper->deleteLater();
+    }
This could be avoided by just passing this as parent when creating the wrappers

+        tube->requestClose();
+        return;
+    }
+
+    TubeWrapper *wrapper = 0;
+    
The last line contains extra EOL spaces

Please add accessors for "does it require credentials and is using unix sockets" and 
rename const TcpSourceAddressGenerator *generator() const; to something more sensible like
tcpGenerator();

Also I think we should default to accept as unix without credentials. 
+    Q_ASSERT(isRegistered());
The current code will crash if onInvokedForTube is called and setToAcceptAs* hasn't being called yet or registerClient failed for some reason.

URL updated to point to the correct branch

(In reply to comment #5)
> Initial review for current branch state. More to come later.
> 
> +    friend class StreamTubeServer; // interim until STS is implemented using
> tube wrapping
>      TELEPATHY_QT4_NO_EXPORT SharedPtr<RefCounted> _object() const; // TODO:
> turn this into _object()
> Is this still needed?
> 

No :) Just forgot to remove it earlier...

> ++++++ simple-stream-tube-handler
> 
> +        foreach (StreamTubeChannelPtr tube, mTubes.keys()) {
> const StreamTubeChannelPtr &, not a ConstPtr

Huh, ConstPtr? But changed to a ref now anyway.

> +        foreach (QString service, p2pServices)
> const QString &
> 
> +        foreach (QString service, roomServices)
> same here

Refs now too, and so are all other non-primitive foreach loop variables where the macro implementation allows it (i.e. not QPairs)

> 
> +        Q_ASSERT(!p2pServices.isEmpty() || !roomServices.isEmpty());
> I don't think we should ASSERT here. Just fail ::create and warn if they are
> both empty.
> This is internal API, but it's called directly from STubeClient/Server
> constructors with user provided params.
> 

My bad, I thought I made the public STS/STC create methods do the check and return null. However, I don't want to return null from the private class create(), because that'd just complicate the public classes' ctors and duplicate the check. And actually now that I think of it, there's a valid usecase for a StreamTubeServer which has an empty filter, which is if you only use it as the PreferredHandler. Thus, I moved the check to STC, which now returns a null StreamTubeClientPtr.

> 
> +            const QString channelType =
> +                chan->immutableProperties()[QLatin1String(
> +                        TELEPATHY_INTERFACE_CHANNEL
> ".ChannelType")].toString();
> This reminds me we should parse immutable properties on construction and
> properly return them on accessors when available. So you could use
> chan->channelType() directly here, even if the channel is not ready.

Yeah but we don't :( This would be a minor performance win for Channel initial state download, but that's a job for another branch. Even more importantly, StreamTubeChannel should initialize itself from the immutable props, so it could avoid the Core GetAll completely - which it currently does unconditionally. Added a TODO anyway to use channelType() here.

> 
> +void SimpleStreamTubeHandler::onReadyOpFinished(Tp::PendingOperation *op)
> +{
> +    Q_ASSERT(!mInvocations.isEmpty());
> +    Q_ASSERT(!op || op->isFinished());
> Well I guess this will break when called from here:
> +        invocation->message = QLatin1String("Got no suitable channels");
> +        onReadyOpFinished(0);

It doesn't. This codepath is thoroughly tested. As a whole SSTH is 97.0% line coverage at the moment, and is missing just testing a tube getting invalidated between it having been prepared and other parts of the PendingComposite for it finishing.

I don't know you think it would break, but I guess because of checking op->isFinished. That is avoided due to the standard C and C++ rule of short-circuit evaluation for boolean logic - because !op, the latter argument to || is not evaluated at all. We make use of this excessively in a whole lot of places...

> Also here you should properly check that context->setFinished is being called
> when
> onReadyOpFinished(0) is called.

We can't check that a context finishes because of the QDBus bug where local loop async replies are ignored. But well, it is, because we set an invocation error.

> You could simplify this by just calling ctx->setFinished/WithError directly
> when no tube channel was provided in handleChannels()

Then the invocations don't finish in order. Currently, the queued finish is used for everything, which maintains perfect event ordering event for these corner cases.

> 
> +        foreach (StreamTubeChannelPtr tube, invocation->tubes) {
> +            if (!tube->isValid()) {
> const StremTubeChannelPtr &, not a ConstPtr
> Please check other loops also for similar "issues".

Umm, again, I don't know what you mean by ConstPtr, but fixed along with the other foreaches.

> 
> Also it seems InvocationData leaks here.

Care to elaborate? All InvocationData pointers are put directly to a RAII SharedPtr when creating them, so they can't leak. Also, valgrind shows no memory leaks from any tp-qt4 code at least in the current StreamTubeHandlers tests, which is, again, already at 97.0% coverage for SSTH.

> 
> Even SimpleStubeHandler being internal I would like to have header guards
> there.

Sure, just seem to have forgotten them.

> 
> 
> ++++++ stream-tube-client
> 
> +    foreach (TubeWrapper *wrapper, mPriv->tubes.values()) {
> +        wrapper->deleteLater();
> +    }
> This could be avoided by just passing this as parent when creating the wrappers

Ah indeed, good idea. Done.

> 
> +        tube->requestClose();
> +        return;
> +    }
> +
> +    TubeWrapper *wrapper = 0;
> +    
> The last line contains extra EOL spaces

Fixed, thanks for spotting

> 
> Please add accessors for "does it require credentials and is using unix
> sockets" and 

There is acceptsAsUnix, and the tubeAcceptedAsUnix signal carries the info on whether the credentials are needed already along with the credential byte which was generated if they are. This is the only credential-requiring information which makes sense; for connecting to a tube, it matters what the STC's settings were when a tube was accepted, not what the settings for future tubes are at the time an application gets the notification for a tube being accepted.

> rename const TcpSourceAddressGenerator *generator() const; to something more
> sensible like
> tcpGenerator();

Renamed

> 
> Also I think we should default to accept as unix without credentials. 

We can't "default" to anything, because as soon as we register the handler object, we can be invoked. If we set ourselves to accept as Unix upon construction and registered ourselves, if the user actually wanted TCP sockets or Credentials AC, they'd need to know to export however they want at the exact same mainloop iteration as creating the STC, otherwise the first (few) channel(s) might be incorrectly accepted. Alternatively, if we just "defaulted to accepting as unix" but didn't register, we'd need an explicit register call, at which point defaulting serves no purpose - no few API calls needed, but it's just needlessly obscure which the settings are in the "default" case.

> +    Q_ASSERT(isRegistered());
> The current code will crash if onInvokedForTube is called and setToAcceptAs*
> hasn't being called yet or registerClient failed for some reason.

It is not called in that case, because the handler is not registered on the bus (so it can't receive channels and consequently emit onInvokedForTube). If that nevertheless happens, it's a bug in SSTH (it e.g. prematurely registers itself or spuriously emits false events) - which is exactly what I tried to catch here with this assert. Added comments explaining this shortly to these asserts in STS and STC.

Branch now updated with 9 commits fixing issues reported in the previous review. I'm going to expand the test coverage for STC further next.

Code-wise, done. Verified using using STS and STC in real-world successful ports of krfb and krdc respectively away from their (somewhat faulty, and very much more verbose) low-level tube code.

Test coverage, also perfect. That is, 80-100% for all parts of the code here.

STS now has full doxygen docs too. Pending documenting STC, which should also be done today.

So ready for final review, fixes and merge before anybody reads this. I'm adding the guy who first and foremost should read this and do something about it then to CC though :P

(In reply to comment #9)
> STS now has full doxygen docs too. Pending documenting STC, which should also
> be done today.
> 
 
Done now. Andre, go go.

Actually done already since tp-qt4 0.7.4 :)