[Bug 54287] Implement Stream Tubes over ICE-UDP
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Mon Sep 3 15:47:35 CEST 2012
https://bugs.freedesktop.org/show_bug.cgi?id=54287
Simon McVittie <simon.mcvittie at collabora.co.uk> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |olivier.crete at ocrete.ca,
| |youness.alaoui at collabora.co
| |.uk
--- Comment #4 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-09-03 13:47:35 UTC ---
Cc Youness, Olivier for their protocol design expertise.
There should be a XEP-style document in docs/ describing how to implement
Stream Tubes as a Jingle application, in the style of XEP-0234 or XEP-0247.
(Indeed, I would like to submit it for standardization, but that's not
necessarily on the critical path.)
When this document has been written and looks basically sane, we should upload
it to a location similar to its own namespace (xmlns) URL, and serve a pointer
to it from its own namespace URL, so that it's somewhat self-documenting. (Just
like I did for the original Tubes.)
At the very least, there should be a document with some correct example stanzas
illustrating the general flow (offer a tube, accept a tube, open two
connections[1] to an accepted tube) and "FIXME" for the rest of the text...
Here is my attempt to reverse-engineer the protocol, feel free to use this in
writing the pseudo-XEP.
--------------
When a tube is offered, the offering peer initiates a Jingle session with a
<description/> in the namespace
<http://telepathy.freedesktop.org/xmpp/tubes-ice>.
The <description/> contains mandatory <service/>, <type/> and <id/> elements,
and an optional <parameters/> element.
<service/> contains a service name appropriate for use with TCP, in the sense
used by the IANA Unified Service Name and Port Number Registry (e.g. "http",
"rfb").
<type/> contains "1" (the numeric value of TP_TUBE_TYPE_STREAM, in ASCII
decimal).
<id/> contains a base-10 ASCII integer in the range 1 to 2**32-1 inclusive.
Stream tubes can be uniquely identified by the pair (peer's full JID, content
of <id/>).
Offering a tube to a peer shares a service with that peer. It is analogous to
listening on a TCP port with listen() and accept().
"Accepting" a tube is purely an XMPP action and has no TCP equivalent. It
indicates that the accepter supports the stream tube protocol and wishes to
connect to the shared service. After accepting a tube, the accepter can
"connect to the tube" (analogous to connect() to a TCP port) up to 128 times.
Each connection has an identifier in the range 0 to 127.
Payload data for all connections to the tube is interleaved through a single
Jingle content with a single component, using the following framing protocol:
* 1 byte: the connection identifier, C
* 2 bytes: the length of the frame, N, as an unsigned integer in either
big- or little-endian. If 0, this frame is a "control frame" as described
below, and does not contain any payload data.
* N bytes: the next N bytes of payload from connection C
Implementations may send frames of any length in the range 1 to 65535. The
boundaries do not have to be preserved and must not be relied on.
In addition, frames with N=0 are "control frames". There are two types of
control frame, distinguished by their connection ID:
* 0 <= C <= 127: end of connection with identifier C
* 128 <= C <= 255: beginning of connection with identifier C
The meaning of frames with N > 0 and C > 127 is undefined.
Candidates whose protocol is UDP indicate use of "pseudo-TCP" compatible with
Google libnice (FIXME: read libnice and document what that actually means),
over ICE-UDP.
Candidates whose protocol is TCP are not currently supported, but in future
they should be. They will operate in the obvious way.
[1] This is explicitly allowed, to support "payload" protocols that are
conceptually connectionless, like HTTP.
--------------
Review comments on that description:
> http://telepathy.freedesktop.org/xmpp/tubes-ice
I think this should be http://telepathy.freedesktop.org/xmpp/stream-tube or
something (with urn:xmpp:jingle:apps:stream-tube:0 requested during
standardization). There are two things wrong with this choice of namespace:
* Stream tubes and D-Bus tubes have different semantics, so they should
be defined as two separate Jingle "applications" each with its own
namespace.
* There's nothing to say that this version of stream tubes *has* to
use ICE. What distinguishes them from the older stream-initiation
tubes is that they're a Jingle application instead of using
stream initiation.
> <type/> contains "1" (the numeric value of TP_TUBE_TYPE_STREAM,
> in ASCII decimal).
Telepathy-specific enums shouldn't be part of our XMPP extension, and in any
case, you should be able to tell it's a stream tube from the namespace of the
<description/> instead.
> <id/> contains a base-10 ASCII integer in the range 1 to
> 2**32-1 inclusive.
I think this should be an arbitrary string (in examples use a UUID for this),
generated by the offerer.
> Payload data for all connections to the tube is interleaved
> through a single Jingle content with a single component,
> using the following framing protocol
Why is there a framing protocol? Why isn't each connection a new content or a
new component?
For instance, we could define it like this:
* offerer adds a dummy content with some sort of special <description>,
just to keep the Jingle session alive; it will never actually transfer
payload data
* accepter sends a content-add per connection
* offerer sends content-accept when connect() to the service has succeeded
or content-reject if it fails
* (optionally) offerer may remove the dummy content as an equivalent of
stopping listen()ing, which would solve Bug #13272
Alternatively, I've also considered a two-layer protocol which would make
stream tubes in multi-user chat consistent with the 1-1 case that you're
dealing with here:
* offerer sends a non-Jingle <iq> (for the 1-1 case) or does some magic
in its <presence> (for the MUC case)
* accepter initiates a Jingle session per connection, with one content,
and a reference to the tube advertisement <iq> in its <description>
One advantage of the framing protocol is that you only have to do one ICE
handshake for all n connections. If we keep the framing protocol, I would like
to make the arbitrary number of connections larger (perhaps just using 2-byte
connection IDs would be sufficient).
> * 2 bytes: the length of the frame, N, as an unsigned integer in either
> big- or little-endian
This is clearly not right: an x86 peer and a PowerPC peer will fail to
communicate. If we're using a framing protocol like this, we have to pick an
endianness. Big-endian is conventional.
----------------
Drive-by review comments on the implementation (not necessarily complete):
Some of your lines are pretty long. Please break argument lists, etc., before
80 characters where feasible, according to the guidelines in
<http://telepathy.freedesktop.org/wiki/Style>.
jingle-tube.c should be jingle-stream-tube.c, with the class renamed
accordingly. If it turns out D-Bus tubes can share code, we can extract a
superclass.
jingle-*.c are meant to be basically ready to extract into Wocky, so they
shouldn't include things that are Telepathy-, D-Bus- or Gabble-specific. In the
case of jingle-tube.c, its use of connection.h is suspicious: it shouldn't need
that.
> + * <description xmlns='http://telepathy.freedesktop.org/xmpp/tubes-ice/stream'>
> + * <service>webdav</service>
> + * <id>666</id>
> + * <parameters>
> + * <parameter name='u' type='str'>anonymous</parameter>
> + * <parameter name='p' type='str'>password</parameter>
> + * <parameter name='path' type='str'>/plots/macbeth</parameter>
> + * </parameters>
> + * </description>
This is not consistent with the implementation. The xmlns has changed, and the
<type/> is missing. My suggested protocol changes actually make it closer to
this example again :-)
> + priv->id = g_ascii_strtoull (id_node->content, NULL, 10);
This is made irrelevant by my suggested protocol changes, but anyway:
priv->id is a guint32, so you're losing the top 32 bits. You should assign the
result of g_ascii_strtoull to a guint64, range-check, and *then* (assuming
success) assign to the guint32.
> + guint32 tmp = self->priv->id;
> +
> + if (tmp == 0 || tmp > G_MAXUINT32)
> + {
> + DEBUG ("tube id is non-numeric or out of range: %u", tmp);
> + return FALSE;
> + }
> + *tube_id = tmp;
This is made irrelevant by my suggested protocol changes, but anyway:
tmp is of type guint32. How can tmp > G_MAXUINT32 fail to be true? (Answer: it
can't.)
This range-checking should have happened while setting id, with the fixes above
too.
> + desc_node = wocky_node_add_child_with_content (content_node, "description", NULL);
> +
> + desc_node->ns = g_quark_from_string (NS_TUBES_ICE);
You should be able to replace NULL with NS_TUBES_ICE in the first line, and
then not need the second line. Even better, use wocky_node_add_build() and
similar helpers, like this:
wocky_node_add_build (content_node,
'(', "description", ':', NS_TUBES_ICE,
'(', "service", '$', type_str, ')',
'(', "id", '$', id_str, ')',
...
')',
NULL);
> + /* At the moment, we don't support DTubes. This should
> never happen, but just in case, return. */
You don't get to say "this should never happen" about things that come from the
network, and in this case, the tube type comes directly from the network. With
my suggested protocol changes, this wouldn't be an issue, because the tube type
would be implicit in the namespace.
> + case TP_CONNECTION_STATUS_CONNECTING:
> + g_signal_connect (conn->jingle_mint,
> + "incoming-session",
> + G_CALLBACK (new_jingle_session_cb), self);
This signal is never disconnected. At least use tp_g_signal_connect_object().
> + len = GPOINTER_TO_UINT (g_hash_table_lookup (priv->length_for_buffers, li->data));
> + if (!gibber_transport_send (transport, (const guint8 *) li->data, len,
> + &error))
This list+hashtable is a really strange data-structure...
> + size = *((guint16 *) &buffer[current_position + 1]);
This is an unaligned access about half the time. It will fail on many non-x86
CPUs, sometimes with SIGBUS or sometimes with silent data corruption. You could
avoid the alignment problems while also fixing the endianness, with:
size = (256 * ((guchar) buffer[current_position + 1]) + (guchar)
buffer[current_position + 2]);
(or the opposite if you standardize on little-endian).
You're also overrunning the end of the buffer if you receive exactly 1 or 2
bytes of the partial message.
I think what you actually want to do is to replace all this TubePartialData
stuff with a simple GByteArray containing unprocessed bytes, and do this:
read()
append all data from the read to the array
pos = 0
/* pop complete messages until we run out of bytes */
while (array->len >= pos + 3)
{
connection ID = array[pos]
size = from_big_endian (array[pos + 1 ... pos + 2, inclusive])
pos += 3
if (size + pos > array->len)
break;
dispatch message (connection ID, size, array[pos ... pos + size - 1,
inclusive])
}
/* discard processed bytes, move unprocessed bytes to the beginning */
g_byte_array_remove_range (array, 0, pos);
--
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
More information about the telepathy-bugs
mailing list