Review for gkiagia's krfb port to StreamTubeServer

[Olli Salli]

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

On 25.09.2011 23:35, George Kiagiadakis wrote:
> On Sun, Sep 25, 2011 at 9:26 PM, Olli Salli <olli.salli at collabora.co.uk> wrote:
>>
>> 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:
> 
> I don't fully know when start() can fail, as it just calls some
> functions from libvncserver to initialize the screen and start
> listening, then checks if the screen is active. It is most likely that
> it will fail if one of the socket functions fails, that's why I added
> the code to try different ports, but if it fails for some other reason
> (not sure what), I wouldn't like it to try thousands of times before
> it gives up.

Right, makes sense. You could poke around its code slightly to see what
it's about? And measure if it takes a significant amount of time at all
to try and start listening at, say port 1 (which can't be done if you're
not root), 16384 times.

If it can fail for other reasons, some of which are costly, perhaps
increasing it to 50-100 or so would be enough to ensure it usually
works. Then it's significantly worse luck already even with e.g.
bittorrent running to have it fail, but it wouldn't still take ages to
do so if failing for other reasons.

A failure code from start() would of course be an even better solution,
as we can change the other code too. You could add a way for it to tell
you that it was bind() which failed? In which case we could continue
trying other ports, otherwise stop right away (because it's not going to
help even up to 5 attempts).

> 
>>
>>>    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).
> 
> To be honest I didn't realize tcpConnections() was there (this line in
> the header file is unreadable...) and I just reused the old code that
> was in krfb. Now that you mention it, I could probably use
> tcpConnections(), although I don't find much value in it. I don't need
> the account pointer and also I don't need to keep track of the source
> address (why do we have this anyway, isn't it always localhost?).

Currently it's always localhost, yes, but the D-Bus API allows
signalling other host addresses as well, so to not restrict
forwards-compatibility wrt utilizing that possibility, we have to
include the host address in the tp-qt4 API as well.

> 
>> 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.
>>
> 
> Maybe a small struct would make it more clear, yes. I don't much like
> pair return values, since it is not very clear what you are doing if
> you use one of its members directly, for example:
> 
>    setContact(sts->tcpConnections()[qMakePair(QHostAddress::LocalHost,
> port)].second);
> 
> is not as clear as:
> 
>    setContact(sts->tcpConnections()[qMakePair(QHostAddress::LocalHost,
> port)].contact);
> 
> and I would have to write something like this to make it more clear:
> 
>    ContactPtr contact =
> tcpConnections()[qMakePair(QHostAddress::LocalHost, port)].second;
>    setContact(contact);
> 

Right, thanks for the feedback there. I'll derive a trivial struct
Tp::StreamTubeServer::RemoteConnectionContact from QPair<AccountPtr,
ContactPtr> with accessors account() and contact() returning the
corresponding QPair members (but more readable). Then it's the best of
both worlds, don't you think? I'll add a placeholder pimpl for the
struct, so we can later add more info for the contacts if that's
available from DBus.

Do you think we need a similar struct for the source address pair (key
of these mappings) too? IMO, that wouldn't make code looking up stuff
from these mappings any cleaner though, as you can already see from an
address and port being passed what those pair members are, so I guess
this is more beneficial for the mapping value types.

> Regards,
> George
> _______________________________________________

Looking forward to reviewing the final version (and the krdc port),
Olli
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJOgdtZAAoJEAQQkupGanj41OQIAKsXTfNhlv8IhEDVvg01IAcz
9ubWJATgwbBf25fhg6Y2L7jsbJwDh3E0IIn5HLgAdMIwg7JDFu4n+me3Rzbyf4Qa
r+iHbWznNFJyEDNu0ZbC7twMkbEeequT/71hG67fUMn1vJhwCLmCxT3gQgiTsvkx
uZUkKfoC/IcFO25UU/7dabfAovh4lGIOicytsT9UTOVvEH/nm5hpWc+uSEdtZ5sP
nqzmaKJhmWdH0xhlUL0Wo7nt+sfKU1Xbcb/GWqZvb/Bq+NhKzoE77SM0H15t1dga
wKGYYqXFBYGcuIPepcK6uFXQyQiLyoH/iUA6of74pFTiGXwp/QdYJEESKbRM2Vw=
=ey7f
-----END PGP SIGNATURE-----