Review for gkiagia's krdc port to Tp::StreamTubeClient

[George Kiagiadakis]

On Fri, Oct 7, 2011 at 5:41 PM, Olli Salli <olli.salli at collabora.co.uk> wrote:
>>     connect(m_stubeClient.data(),
>> SIGNAL(tubeAcceptedAsTcp(QHostAddress,quint16,QHostAddress,quint16,
>
> Yeah, this is all that's needed for basic operation. But I think you
> should also connect to tubeClosed to clean up from your hash tubes which
> WE don't close, but get closed for other reasons. E.g. server connection
> reconnect, or the remote going away.
>
> Or are you confident that closeTube will always get invoked in all those
> occasions via some other mechanism? (Likely: getting disconnect event /
> error on the server socket - but would that really call the functions to
> INITIATE a disconnect like disconnectHost or closeTab?)

I think you are right, I should use the signal. Although I think it
should work fine like this, because the closed socket will cause a
disconnect, which will close the active tab (although this actually
requires a patch that is not in krdc yet; in the current situation the
tab is not closed automatically, which leaves krdc in a non-consistent
state)

> You might at this point want to investigate using the parameters for
> passing some kind of credentials to avoid accidental / undesired access
> to the rfb socket, as we discussed in the krfb review. Setting them when
> exporting the socket in krfb, you could recover them tube->parameters()
> in your onTubeAccepted() slot, then use to do rfb authentication or
> whatever (as much as that's supported by the protocol libraries used).

Yeah, I should do that, although in the current design of krdc this is
not so easy. It will require some extra changes in other parts of
krdc.

> Otherwise looks perfect - and wonderfully simple indeed, thanks to
> StreamTubeClient.
>
> I think we're now in a point where implementing tubes enabled software
> is pretty much easy enough code-wise for broader adoption. However,
> understanding enough of the spec to write the .client files still
> remains an issue, though (not visible in this patch, as that was already
> done for the old code, however). Maybe we should add some API to
> StreamTubeClient/StreamTubeServer, which would write to a given
> QIODevice the .client file corresponding to what services they were
> created for, without even having to export with it / set it to accept
> tubes? One could then write a tiny application to hook this to their
> build system. Opinions?
>
> Another option is generalized .client file writing from introspecting
> running Clients, but that'd require a dbus session, potentially GUI if
> the tubes code isn't separated enough to its own module etc... so I
> think that's not as useful, and would be a lot more involved.
>
> Or maybe just a simple python script, which you give --p2pservices
> rfb,ftp --roomservices ftp or alike, and it then fills a template which
> it prints to stdout? Simplest of them all, but unified way to create the
> handler for generating the file and then for actually registering and
> using it would be nice. That'd also generalize better if we add more
> attributes besides the tube service name to STC/STS creation parameters
> affecting the resulting filter. Dunno...

I think that a simple script would be enough. Maybe together with some
nice cmake macro which will be exported in TelepathyQt4Config.cmake
(which I hope to have ready for merge soon...)

Regards,
George