[Bug 38206] Tp::StreamTubeChannel (and subclasses) don't have unit tests

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=38206

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |http://cgit.collabora.com/g
                   |                            |it/user/andrunko/telepathy-
                   |                            |qt4.git/log/?h=stubes

--- Comment #3 from Olli Salli <ollisal at gmail.com> 2011-06-28 04:22:40 PDT ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.