[Bug 29218] TpStreamTube - high level stream tube API

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=29218

--- Comment #31 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-10-01 08:51:23 PDT ---
The general shape of the API looks fine. Some notes about things I happened to
spot:

> +static void
> +tp_stream_tube_channel_constructed (GObject *obj)

I noticed this doesn't chain up. It should do that first.

> +   * @stream: (transfer none): the #TpStreamTubeConnection for the connection

I don't think (transfer) is applicable to objects in signals: the signal
machinery always takes one ref for the duration of the signal emission, and the
recipient needs to ref it if they want to keep it. It's worth mentioning it
explicitly in the text though, like you did.

> +  if (n_failed > 0)
> +    {
> +      DEBUG ("Failed to prepare TpContact (unspecified error)");

If error == NULL and n_failed > 0, that unambiguously means the failed handles
failed with InvalidHandle. Improved docs wording would be welcome.

We ought to introduce a signal with contact IDs to avoid the possibility of
InvalidHandle, really...

> +        /* FIXME: set the address of the socket. Gio doesn't seem to have API
> +         * to get the port before connecting without specifying the whole
> +         * adress (we can't as we don't know which ports are available). */

You have to do a GIO-flavoured version of this pseudocode:

sock = socket (AF_INET, SOCK_STREAM, 0);
bind (sock, addr={ AF_INET, sin_port=0, sin_addr=IN_ADDR_ANY }, addr_len);
getsockname (sock, &real_addr, real_addr_len);
/* time passes */
connect (sock, blah blah blah);

I think the GIO version of that might be to make the GSocketClient earlier,
call g_socket_client_set_local_address() for IN_ADDR_ANY with port 0, then call
g_socket_client_get_local_address() to find out what the kernel actually gave
us?

> +      /* Pass the ref on tube_conn to the callback */
> +
> +      tp_connection_get_contacts_by_handle (
> +          tp_channel_borrow_connection (TP_CHANNEL (self)),
> +          1, &handle, 0, NULL,
> +          _new_remote_connection_with_contact,
> +          tube_conn, NULL, G_OBJECT (self));

You can't rely on the callback being called if you pass a weak object - if
@self dies, the callback won't happen. If you just want to unref tube_conn in
the "@self died" case, you can pass g_object_unref as the destructor here
instead of releasing the ref in the callback?

> +  g_assert (self->priv->connection != NULL);

TP_IS_CONNECTION?

> + * tp_stream_tube_connection_get_connection: (skip)

I wonder whether tp_stream_tube_connection_get_socket() would be better, given
that TpConnection is a prominent part of our API? :-)

_tp_create_temp_unix_socket looks like an ideal candidate for putting in GIO.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.