Review for gkiagia's krfb port to StreamTubeServer

[Olli Salli]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

George has a branch at
http://cgit.collabora.com/git/user/gkiagia/krfb.git/log/?h=new-tubes-api, but
there's no corresponding merge request (and krfb is not really one of
our projects... in theory, at least)

I'm going to focus on the TP side of things, as I don't really know the
krfb internals.

- --

    setPasswordRequired(false);

For added security, you could generate a random password, and export
that as a tube parameter via the optional params parameter to
exportTcpSocket. The other end (krdc/alike) would pick this up from its
IncomingStreamTubeChannel::parameters() and authenticate with it using
the RFB protocol. (Using StreamTubeClient in the other end, this can be
done by inspecting the tube channel from the tubeAcceptedAsTcp signal).

This is only anyway locally exploitable as your socket listens on local
loopback, but still anybody from the computer, including different
users, can connect to it, which might be undesired for some terminal
server -like uses (think remote access to your account on some box -
you'd run krfb there, but without a password anybody else having access
of their own to the box could access your session!).

- --

> void TubesRfbServer::onTubeRequested()
> {
>     kDebug() << "Got a tube";
> }

> void TubesRfbServer::onTubeClosed()
> {
>     kDebug() << "tube closed";
> }

tp-qt4 already emits you plenty of debug for the tubes being requested
and closed (including the object path, close error etc) so if you aren't
going to do anything on those events, I'd just drop these slots altogether.

- -- 

>         //try a few times with different ports
>         bool ok = false;
>         for (int i=0; !ok && i<5; i++) {
>             setListeningPort((KRandom::random() % 16383) + 49152);
>             ok = start();
>         }

>         if (!ok) {
>             kError() << "Failed to start tubes rfb server";
>             return;
>         }

OK, I know this is not really TP, but giving up after trying a few
random ports seems a bit shady. I'd also like the error message to be
more explicit about the cause.

Is the port being already taken the only reason why start() could fail?
Is start() a particularly heavy operation, or is it pretty much just
socket(), bind() & listen()? If it's cheap and (at least usually) fails
because the port is taken, I'd rather just randomize the starting port,
then try every successive port after that (and rolling over on 65535),
and only stop when all 16384 ports have been tried. Something like:

bool ok = false;
int base = KRandom::random() % 16384;
for (int i = 0; !ok && i < 16384; i++) {
    setListeningPort(49152 + ((base + i) % 16384));
    ok = start();
}

(Note that i % 16384 is in range [0, 16383] for all integers i). Even if
all ports are taken, this shouldn't take too many milliseconds if
start() is mostly just those socket operations.

Randomized starting point, though so that if a lot of apps do the same,
they wouldn't all have to first try out 49152, 49153, ... - in most
environments they'd operate like your current code and be successful
with the first or so port (the base), but wouldn't fail as easily when a
significant fraction of the ports are taken (can easily happen with e.g.
bittorrent with many allowed outbound connections).

 --

>d->stubeServer->exportTcpSocket(QHostAddress(QString::fromAscii(listeningAddress())),
>                                    listeningPort());

You should check that stubeServer->isRegistered(), and at least kError()
or ideally somehow signal to krdc that you aren't actually going to get
any connections (freeing up the listen port etc). This could happen e.g.
because something else has already taken your bus name (another
instance? is there some pre-existing instance activation logic in krfb?)
or you don't have a working dbus connection.

 --

>    QHash<quint16, Tp::ContactPtr> contactsPerPort;
>    d->contactsPerPort[sourcePort] = contact;
>    d->contactsPerPort.remove(sourcePort);
>    if (d->contactsPerPort.contains(port)) {

Do you really need to maintain this mapping yourself?
StreamTubeServer::tcpConnections() gives you the same info - though
granted, it'll be slightly clumsier to look up the contact from as it
has the account too (and you need to include QHostAddress::LocalHost in
the key you look up with). However, if there is some kind of virtual fn
notifying about clients disconnecting you can implement to clean up
clients from the clientsPerPort mapping, you could then do away with
onTcpConnectionClosed entirely.

If you think the return type of STS::tcpConnections is too clumsy, I'm
open to suggestions on how to simplify it (split to multiple accessors?
make some small struct with account/contact rather than using raw
pairs?). We can still change it as the APIs haven't yet been released in
tp-qt4.

 --

In general it seems STS fits the usecase here rather well, which is nice
to know.

Br,
Olli
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJOf3JXAAoJEAQQkupGanj4rs8H/irxTRoeJSvrOHsFZ3OBNpqZ
W02wGuHYLHpVdTSgzjP8cGHbluxPw7bDlniAY82G4k8GqHeUoC8cggtvuN7uaGMj
qcPSihx3vuYsfx5B+E/8gMHOMlx3NapwtuA6N79EWKO7kr6pnqVc+Irf4hxcLFA7
Bczd/++3gTWbTMoGgT8DtudijgSoDjTDu6KlZVo7WXjytlUEUFNWL7cwQenN/iMU
pnJ3yYILQYwbmBrNZp4Twmnu1hEUKrISwSdNQsOqRxgHAtCWDsE8hOa+ZMy3LLHA
vOlccA9IYEBBJjTkAl96qLjtnD1lzI5E7WurFfQB+cJHcaU3vh4D0WVmbbSmvkU=
=kYQo
-----END PGP SIGNATURE-----