[Telepathy] Re: Spec changes involving FileTransfers and AppConnect channels

[Simon McVittie]

Here at GNOME Summit there seems to be interest in doing crazy
collaborative things over Telepathy :-) so I'm having a look through your
changes and trying to get them reviewed and merged into Gabble Darcs.

I haven't looked at your implementation in Gabble yet, so some of my
criticisms here may be somewhat wrong, but after the long delay I
thought I'd better get you *some* feedback as quickly as I can! As I've
only just got involved in this, I'm missing a lot of context, so I'm
going to have to read up on the JEPs and use cases too.

As you mentioned in your email back in August, the file transfer
interface where the connection manager opens the file is less than ideal.
Particularly, if you're using some sort of Telepathy equivalent of an
IRC bouncer, it's possible that not all your Telepathy components are
running on the same machine.

Mads Chr. Olesen wrote a patch which included:
> +    def RecieveFile(self, filename):

ITYM "Receive".

> +        Returns how much of the file has been transferred, in bytes (0 - 2^64).

ITYM "0 - 2^64-1".

> +class ChannelTypeAppConnect(Channel):
> +    """
> +    A channel for connecting an application at the local host, with an
> +    application on a handle's remote host, through a socket connection.

... "on a contact's (remote) host", ...

We should be careful to make it clear which type a handle is, since
they're all just integers.

> +    @dbus.service.method(CHANNEL_TYPE_APP_CONNECT, in_signature='ss', out_signature='')
> +    def Propose(self, app, socketaddr):
> +        """
> +        Sets what application we want to connect to, e.g. "Gobby",
> +        and which UNIX socket address the local application is listening at, e.g.
> +        "/tmp/gobby1". The channel will then start establishing the connection between
> +        the two endpoints. When a connection is made to the local socket, communication
> +        can start (the Connected signal will be emitted at the same time).

Some thoughts:

What's the scope of this extension? Do you intend to support arbitrary
reliable streams, or just P2P D-Bus? Or do we even want to support
unreliable and/or packet-oriented connections, like UDP?

If it's arbitrary reliable streams, in a way it'd be nice to be able to
reuse some of the infrastructure used for streamed media, but specifying
a reliable (TCP-like) stream.

If the connection manager and the UI are running on different hosts the
Unix socket won't actually work... if this is something we want to
support at all.

Rob Taylor points out that our design has always been that connection
managers never open files, so we can run them "jailed" using SELinux.
Using Unix domain sockets is less than ideal for this.

(It would be great if you could transfer a fd over D-Bus, but this is
sadly not the case.)

If we *do* want to use Unix domain sockets (and for authentication
purposes, they're a nice thing to be using), and we're restricting the
communication protocol to be D-Bus, instead of a filename we
should be using a (list of) D-Bus connection description(s) in
the same format you'd find e.g. in the DBUS_SESSION_BUS_ADDRESS
environment variable, such as
"unix:abstract=/tmp/gobby.blahfoo;x-tcp:addr=127.0.0.1,port=12345".

> +        appname - the application's name
> +            TODO: This should be a D-Bus object path, for D-Bus activation, etc., depending
> +            on how mission control shapes out.

We could certainly do with some sort of namespacing for this, rather
than just having it be some random string.

> +    @dbus.service.method(CHANNEL_TYPE_APP_CONNECT, in_signature='', out_signature='s')
> +    def Accept(self):
> +        """
> +        Accepts an incoming AppConnect request, telling the local application
> +        which socket to connect to. The application should not begin connecting
> +        to the local socket before the Connected signal has been emitted. 
> +        Can only be called once, to minimize the risk of the channel being 
> +        handled multiple times!
> +
> +        Returns:
> +        the UNIX socket address of where the local application should connect,
> +        to communicate over this AppConnect channel, e.g. "/tmp/appconnchannel1"
> +
> +        Possible Errors:
> +        Disconnected, NotAvailable

Can we have some description of when each error is raised? I realise the
rest of the spec isn't very good about this either. In particular, if
you're saying that calling Accept twice is erroneous (which it is), it
should be documented what gets raised in that case. When I get round to
reading your implementation patches I'll reverse-engineer the error
behaviour and write it up in the spec!

Regards,
	Simon