Bug 38206

Andre is working on this stuff. He has a WIP branch at the URL - I'm going to drop some quick review comments now. He has found some bugs too while testing the code, and has fixes for those.

> StreamTubeChannel: Make introspectable do not depend on Channel.Type.StreamTube.
> StreamTubeChannel: Fix supportsUnixSocketsWithCredentials.
> TubeChannel: Fix typo in debug message.

++

+    bool initRandom;
This could perhaps be a static variable. The qsrand function sets global state; we don't need to do that for each tube channel separately.

+bool PendingStreamTubeConnection::requireCredentials() const

This method name is an imperative; and gives the hint that it would actually make something require credentials. Instead, it's an accessor which checks if credentials are required. Hence, -> requiresCredentials().

+      g_assert_cmpuint (port, ==,
+          g_value_get_uint (self->priv->access_control_param));

As discussed, you need to fix this spec-incompliant requirement in the test service (both in our copy and in tp-glib). You can file a separate tp-glib bug for this, stating that it incorrectly passes only the port where the spec requires the address and port as a struct, and that's what gabble expects too. If the tp-glib client side is simple to fix, please do that as well. You don't need to depend on the tp-glib version with the fixes though, as we have a local copy of this test service, and the tp-glib copy will be fixed before our next sync from their test services for sure.

The current test looks broadly OK - unfortunately I don't have time to review it in detail yet. However, you must also eventually test the connection monitoring functionality, as it's easily the most complex part of our tube client code (with the async contact builds). This might or might not require extensions to the test service.

You should also check, not only for UNIX sockets, but for TCP sockets too, that the PendingStreamTubeConnection and the tube channel both return a valid address which one can connect to.

(In reply to comment #1)
> +    bool initRandom;
> This could perhaps be a static variable. The qsrand function sets global state;
> we don't need to do that for each tube channel separately.
Done
 
> +bool PendingStreamTubeConnection::requireCredentials() const
> 
> This method name is an imperative; and gives the hint that it would actually
> make something require credentials. Instead, it's an accessor which checks if
> credentials are required. Hence, -> requiresCredentials().
Done
 
> +      g_assert_cmpuint (port, ==,
> +          g_value_get_uint (self->priv->access_control_param));
> 
> As discussed, you need to fix this spec-incompliant requirement in the test
> service (both in our copy and in tp-glib). You can file a separate tp-glib bug
> for this, stating that it incorrectly passes only the port where the spec
> requires the address and port as a struct, and that's what gabble expects too.
> If the tp-glib client side is simple to fix, please do that as well. You don't
> need to depend on the tp-glib version with the fixes though, as we have a local
> copy of this test service, and the tp-glib copy will be fixed before our next
> sync from their test services for sure.
Done, tp-glib patches will come later when I finish this.
 
> The current test looks broadly OK - unfortunately I don't have time to review
> it in detail yet. However, you must also eventually test the connection
> monitoring functionality, as it's easily the most complex part of our tube
> client code (with the async contact builds). This might or might not require
> extensions to the test service.
> 
> You should also check, not only for UNIX sockets, but for TCP sockets too, that
> the PendingStreamTubeConnection and the tube channel both return a valid
> address which one can connect to.
In progress.

The updated branch now also enables tests for Port access control.

The downside is that I was unable to test Tcp+Port using Qt sockets directly. I used G*Socket* instead, see the test for more details.

Umm, I wonder where the URL went. There it is anyway.

> > The current test looks broadly OK - unfortunately I don't have time to review
> > it in detail yet. However, you must also eventually test the connection
> > monitoring functionality, as it's easily the most complex part of our tube
> > client code (with the async contact builds). This might or might not require
> > extensions to the test service.

Yeah, this. Very important :p

> > 
> > You should also check, not only for UNIX sockets, but for TCP sockets too, that
> > the PendingStreamTubeConnection and the tube channel both return a valid
> > address which one can connect to.
> In progress.
> 
> The updated branch now also enables tests for Port access control.

+      guint16 port, remote_port;
 
-      addr = g_socket_connection_get_remote_address (connection, &error);
+      dbus_g_type_struct_get (self->priv->access_control_param,
+              0, &host,
+              1, &port,
+              G_MAXUINT);

I think this might not work when this lands in tp-glib, or if it does, only 
with little-endian architectures, because AFAIK tp-glib will always just send a 32-bit uint in that variant (incorrectly?). At least Gabble extracts the port to a guint (a 32-bit uint), not a guint16.

Could some dbus-glib expert give some insight here? Namely, is it wrong for Gabble to extract the param to a 32-bit variable? Or is it wrong for us to extract it into a 16-bit variable, in which case the spec needs to be corrected to specify u instead of q as the type of the port variable, as the current spec is unimplementable with dbus-glib?

> 
> The downside is that I was unable to test Tcp+Port using Qt sockets directly. I
> used G*Socket* instead, see the test for more details.

+                // QAbstractSocket::setSocketDescriptor does not work either, so using glib sockets

Really? A quite basic function documented since Qt 4.1 doesn't work?

As for letting Qt create the socket but binding it, did you try the suggested workaround from QTBUG-121, which I guess in code could be something as simple as

sockaddr_in sin;
socklen_t sinlen = sizeof(sin);
std::memset(&sin, 0, sinlen);

sin.sin_family = AF_INET;
sin.sin_addr.s_addr = INADDR_LOOPBACK;
sin.sin_port = 0; // 0 is random free port, or change to htons(some desired one) if you rather want that

bind(sock->getSocketDescriptor(), reinterpret_cast<sockaddr *>(&sin), sinlen);

getsockname(sock->getSocketDescriptor(), reinterpret_cast<sockaddr *>(&sin), &sinlen);

At that point, sin should contain your socket's local address and you can proceed with giving it to the CM and connecting. You can translate the address to a C-string with inet_ntop and the port to a native integer value with ntohs.

Of course, it would be nicer if Qt had the API for this - as this needs different includes on different platforms etc., but we'd need to wait for at least Qt 4.8 for that.

In any case, I'd very much prefer if our GLib could be restricted to the test services as much as possible. If it can't be done, it can't be done, though.

Some other observations:

    QVERIFY(connect(chan->acceptTubeAsUnixSocket(),
                SIGNAL(finished(Tp::PendingOperation *)),
                SLOT(expectSuccessfulCall(Tp::PendingOperation *))));
    QCOMPARE(mLoop->exec(), 1);

You should use expectFailure in these cases, so you don't get a warning.

Oh, one thing you should test for:

1) begin accepting a tube
2) immediately, somehow invalidate the channel (either through client-side backdoor API or some service backdoor API)
3) verify that the accept operation FAILED instead of hanging forever

And as a variation, make it adjustable when the service goes to StateOpen, and only do that a few mainloop iterations after returning from the Accept call. Then in between invalidate the channel. I believe that would hang the current PendingStreamTubeConnection implementation, which needs to start checking and waiting for channel invalidation when waiting for the state change to Open as well. The currently WIP dbus tube branch has the same issue actually.

(In reply to comment #3)
> +      guint16 port, remote_port;
> 
> -      addr = g_socket_connection_get_remote_address (connection, &error);
> +      dbus_g_type_struct_get (self->priv->access_control_param,
> +              0, &host,
> +              1, &port,
> +              G_MAXUINT);
...
> Could some dbus-glib expert give some insight here? Namely, is it wrong for
> Gabble to extract the param to a 32-bit variable? Or is it wrong for us to
> extract it into a 16-bit variable, in which case the spec needs to be
> corrected to specify u instead of q as the type of the port variable, as
> the current spec is unimplementable with dbus-glib?

In dbus-glib, n and q (16-bit D-Bus integers) map to G_TYPE_(U)INT, int-sized types, which in principle might not be 32-bit - although they're 32-bit on all current platforms. The correct C type is g(u)int, not g(u)int16 or g(u)int32.

i and u (32-bit D-Bus integers) *also* map to G_TYPE_(U)INT.

Obviously, this loses information - dbus-glib will map G_TYPE_(U)INT back to i and u. So anything where dbus-glib is meant to send a 16-bit integer will come out as 32-bit on the wire, and yes, that makes q unimplementable (Bug #20776). Unfortunately, nobody wanted to do anything about this before we declared tp-qt4 to be stable.

In the long term, one way or another, someone has to break ABI (either tp-glib moving to GDBus - which can send 16-bit things - or tp-qt4 breaking its low-level ABI to use 32-bit throughout).

In the short term, telepathy-qt4 evades this by using permissive type conversion for various things: if the correct 16-bit version fails, it tries a 32-bit equivalent. Most uses of 'q' are values in an a{sv} anyway, where being picky about the difference between numeric type sizes is unhelpful.

(In reply to comment #4)
> In dbus-glib, n and q (16-bit D-Bus integers) map to G_TYPE_(U)INT, int-sized
> types, which in principle might not be 32-bit - although they're 32-bit on all
> current platforms. The correct C type is g(u)int, not g(u)int16 or g(u)int32.
> 
> i and u (32-bit D-Bus integers) *also* map to G_TYPE_(U)INT.
> 
> Obviously, this loses information - dbus-glib will map G_TYPE_(U)INT back to i
> and u. So anything where dbus-glib is meant to send a 16-bit integer will come
> out as 32-bit on the wire, and yes, that makes q unimplementable (Bug #20776).
> Unfortunately, nobody wanted to do anything about this before we declared
> tp-qt4 to be stable.
> 
> In the long term, one way or another, someone has to break ABI (either tp-glib
> moving to GDBus - which can send 16-bit things - or tp-qt4 breaking its
> low-level ABI to use 32-bit throughout).
> 
> In the short term, telepathy-qt4 evades this by using permissive type
> conversion for various things: if the correct 16-bit version fails, it tries a
> 32-bit equivalent. Most uses of 'q' are values in an a{sv} anyway, where being
> picky about the difference between numeric type sizes is unhelpful.

Thanks for the summary.

I think for the case at hand this means that tp-qt4 can continue sending 16-bit values over the bus as specified, but we just need to change the test service to extract to a guint like gabble does (as dbus-glib will turn the 16-bit value received from the bus to a G_TYPE_UINT).

Then, when the test service is used with tp-qt4, it'll receive 16-bit values which get transformed into G_TYPE_UINTs in the service side. When it's used with tp-glib client side, the cast happens already in the client side, but the end result is the same.

(In reply to comment #3) 
> > > 
> > > You should also check, not only for UNIX sockets, but for TCP sockets too, that
> > > the PendingStreamTubeConnection and the tube channel both return a valid
> > > address which one can connect to.
> > In progress.
> > 
> > The updated branch now also enables tests for Port access control.
> 
> +      guint16 port, remote_port;
> 
> -      addr = g_socket_connection_get_remote_address (connection, &error);
> +      dbus_g_type_struct_get (self->priv->access_control_param,
> +              0, &host,
> +              1, &port,
> +              G_MAXUINT);
> 
> I think this might not work when this lands in tp-glib, or if it does, only 
> with little-endian architectures, because AFAIK tp-glib will always just send a
> 32-bit uint in that variant (incorrectly?). At least Gabble extracts the port
> to a guint (a 32-bit uint), not a guint16.
> 
Will fix it

> > 
> > The downside is that I was unable to test Tcp+Port using Qt sockets directly. I
> > used G*Socket* instead, see the test for more details.
> 
> +                // QAbstractSocket::setSocketDescriptor does not work either,
> so using glib sockets
> 
> Really? A quite basic function documented since Qt 4.1 doesn't work?
> 
> As for letting Qt create the socket but binding it, did you try the suggested
> workaround from QTBUG-121, which I guess in code could be something as simple
> as
> 
> sockaddr_in sin;
> socklen_t sinlen = sizeof(sin);
> std::memset(&sin, 0, sinlen);
> 
> sin.sin_family = AF_INET;
> sin.sin_addr.s_addr = INADDR_LOOPBACK;
> sin.sin_port = 0; // 0 is random free port, or change to htons(some desired
> one) if you rather want that
> 
> bind(sock->getSocketDescriptor(), reinterpret_cast<sockaddr *>(&sin), sinlen);
> 
> getsockname(sock->getSocketDescriptor(), reinterpret_cast<sockaddr *>(&sin),
> &sinlen);
> 
> At that point, sin should contain your socket's local address and you can
> proceed with giving it to the CM and connecting. You can translate the address
> to a C-string with inet_ntop and the port to a native integer value with ntohs.
> 
> Of course, it would be nicer if Qt had the API for this - as this needs
> different includes on different platforms etc., but we'd need to wait for at
> least Qt 4.8 for that.
> 
> In any case, I'd very much prefer if our GLib could be restricted to the test
> services as much as possible. If it can't be done, it can't be done, though.
I tried the other way around, tried creating a socket using libc and glib. bind it and pass it to QTcpSocket using setSocketDescriptor. After that calling connectToServer fails horribly, but I will try what you suggested to see if it works.

> Some other observations:
> 
>     QVERIFY(connect(chan->acceptTubeAsUnixSocket(),
>                 SIGNAL(finished(Tp::PendingOperation *)),
>                 SLOT(expectSuccessfulCall(Tp::PendingOperation *))));
>     QCOMPARE(mLoop->exec(), 1);
> 
> You should use expectFailure in these cases, so you don't get a warning.
> 
> Oh, one thing you should test for:
> 
> 1) begin accepting a tube
> 2) immediately, somehow invalidate the channel (either through client-side
> backdoor API or some service backdoor API)
> 3) verify that the accept operation FAILED instead of hanging forever
> 
> And as a variation, make it adjustable when the service goes to StateOpen, and
> only do that a few mainloop iterations after returning from the Accept call.
> Then in between invalidate the channel. I believe that would hang the current
> PendingStreamTubeConnection implementation, which needs to start checking and
> waiting for channel invalidation when waiting for the state change to Open as
> well. The currently WIP dbus tube branch has the same issue actually.
I will add this too.

Branch updated with some tests for outgoing stubes as well, more to come.

Branch updated again with loads of new changes including some refactoring of parts of the tubes code.

The test to check whether accept fails if the channel is invalidated will come next. Any other tests I should consider?

I am also updating all docs as they were really outdated/wrong and I don't want to merge this code with the docs as they are now.

The offer tests and specifically the checks for the map contents look okay to me.

An API issue:

+ * This method is only useful if TubeChannel::addressType() is either #SocketAddressTypeUnix or
+ * #SocketAddressTypeAbstractUnix and TubeChannel::accessControl() is
+ * #SocketAccessControlCredentials.
+ *

The user doesn't set the addressType() or accessControl(). The user passes true or false as the requiresCredentials param to offerUnixSocket() which they're using. Hence, you must refer to those concepts rather than the lower-level accessors - the user doesn't have any idea how to make this method "useful" otherwise!

The same goes for the warning messages.

For that reason, I don't think it's necessarily a great idea to have a accessControl() public accessor rather than e.g. some passesCredentials() accessor (with the logic that because you told offerUnixSocket() that your socket requires credentials, the CM will pass you credentials when connecting).

In a word, *consistency*.

The above method's description should also follow the general short descr - what it does - when can it be used pattern. Currently the long description begins with the quoted chapter, which I'd like moved together with the features it needs etc, after the actual description of the mapping.

> The test to check whether accept fails if the channel is invalidated will come
> next. Any other tests I should consider?

That's the most important outstanding bit, with the channel invalidated at various different stages of the process.

I'd also like a version of the onNewConnection() slot which checks that when that signal is emitted, the connectionsFor*..., contactsFor.. maps are populated with the information for the new connection. The current test only checks the map contents potentially significantly later 

Oh, you should also probably check that ConnectionClosed is correctly processed and relayed. In that, however, *the maps should still contain the data in the slots for the Qt signal* so you can figure out which connection it was.

Other than that, can't think of much - but have you looked at lcov-check, if any significant portions of the code are still red?

The StreamTube stuff is finally looking rather mature, good work!

(In reply to comment #9)
> The offer tests and specifically the checks for the map contents look okay to
> me.
> 
> An API issue:
> 
> + * This method is only useful if TubeChannel::addressType() is either
> #SocketAddressTypeUnix or
> + * #SocketAddressTypeAbstractUnix and TubeChannel::accessControl() is
> + * #SocketAccessControlCredentials.
> + *
> 
> The user doesn't set the addressType() or accessControl(). The user passes true
> or false as the requiresCredentials param to offerUnixSocket() which they're
> using. Hence, you must refer to those concepts rather than the lower-level
> accessors - the user doesn't have any idea how to make this method "useful"
> otherwise!
> 
> The same goes for the warning messages.
> 
> For that reason, I don't think it's necessarily a great idea to have a
> accessControl() public accessor rather than e.g. some passesCredentials()
> accessor (with the logic that because you told offerUnixSocket() that your
> socket requires credentials, the CM will pass you credentials when connecting).
> 
> In a word, *consistency*.
> 
> The above method's description should also follow the general short descr -
> what it does - when can it be used pattern. Currently the long description
> begins with the quoted chapter, which I'd like moved together with the features
> it needs etc, after the actual description of the mapping.
Ok, moved the accessControl() accessor to be protected now as I believe it may be useful for custom classes (if any). Tbh I don't see a point in adding a passesCredentials() (or I would name it expectsCredentials() - expects credentials from CM) as whoever called offerUnixSocket() already know if credentials are to be expected or not (at least they should). Let me know if you still think this should be added.

Branch updated again with more tests (channel invalidation while accepting, connectionClosed, ...) + some more fixes/updates, I will finish the docs updates next.

(In reply to comment #10)
> Ok, moved the accessControl() accessor to be protected now as I believe it may
> be useful for custom classes (if any). Tbh I don't see a point in adding a
> passesCredentials() (or I would name it expectsCredentials() - expects
> credentials from CM) as whoever called offerUnixSocket() already know if
> credentials are to be expected or not (at least they should). Let me know if
> you still think this should be added.
Ah and addressType() is already public upstream, so no change here.

+    virtual void processConnectionClosed(uint connectionId, const QString &errorName,
+            const QString &errorMessage);
+

We can't add virtual methods to public classes unless we assume nobody has subclassed them. Additionally, on some architectures the vtable layout might change sufficiently to break even the virtual destructor invocation in the SharedPtr<OutgoingStreamTubeChannel> destructor (which is inline, and hence in client code).

So this has to be handled in some other way. I assume your goal here was to keep the removed connection in the mappings until the signal has been delivered to slots in client code? Just using Qt::QueuedConnection for the connect() call in the original OutgoingStreamTubeChannel destructor would accomplish that.

+void PendingStreamTubeConnection::onChannelInvalidated(DBusProxy *proxy,
+        const QString &errorName, const QString &errorMessage)
+{

This could use a warning indicating that the accept operation failed because the channel was invalidated with said error.

+ * When this capability is available, the stream tube can be accepted or offered without any
+ * restriction on the access control on the other end.

What on earth is this supposed to mean? The access control used for the other end never plays any role, no matter what the locally supported access controls.

Otherwise looks good now.

Tnx for the review, changes applied and branch merged upstream. It will be in 0.7.2.