[Telepathy] Review of telepathy-salut.git/tubes
guillaume.desmottes at collabora.co.uk
Mon Aug 18 07:38:17 PDT 2008
This is a first review of
what's the status of 1-1 D-Bus tubes? We should clarify it before
We shouldn't ignore the ack of the SI offer.
Please explain the IP policy when a new tube stream is established.
Tube closing should have examples.
Maybe bytestream-direct-tcp or bytestream-raw-tcp would be a better
header file: args should be 4 spaces aligned
g_value_set_string (value, (const gchar *)"");
we should define and use a name for this
priv->stream_init_id doesn't make sense anymore
Please wrap > 80 chars lines
args of the getter/setter/constructor are miss aligned
args of g_signal_new should be 4 spaces aligned
transport_buffer_empty_cb (GibberTransport *transport,
add a space before this function
gibber_bytestream_direct_initiate: we should store the SalutContact of
the remote peer and use it to get his IP.
Do we still need to store the XMPP connection ?
Why remove the if (!NULL) check when disposing? We always check that in
salut-bytestream-manager.h wasn't removed
/* guint id -> guint listener_watch
* When used by stream tubes, the id is the tube_id */
That's a lie, this hashtable contains
Humm actually, the key doesn't seem to be a guint either but a tube ptr.
I'm not sure this kind of mapping make sense. We just need a way for the
bytestream user to stop listening right? Maybe we could pass the port to
salut_direct_bytestream_manager_stop_listen and store listeners in a
Or maybe having a real listener object could make sense?
priv->listeners = g_hash_table_new (NULL, NULL);
you should_use g_hash_table_new_full and pass a listener_free function
salut_direct_bytestream_manager_stop_listen: shouldn't remove the
listener from the hashtable? They are never removed.
You don't seem to need priv->host_name_fqdn anymore.
shouldn't it return TRUE to continue listening?
you should check if the connection is coming from the right contact
I think you could just use g_source_remove (id)
I'm wondering if, finally, that make sense to have 2 distinct bytestream
mgrs. I think the plan is to factor out most of
salut-direct-bytestream-manager to a GibberTcpListenner object (or
something similar) that could be used by GibberXmppConnection and
probably stream tubes too (to watch connections on the local tube
socket). Maybe then we could use it directly in a unified
bytestream-mgr ? Or maybe if the API of this GibberTcpListenner is easy
enough, the direct-bytestream-mgr won't make sense anymore and we could
use it directly from the stream tube module?
function declarations at the begin of the .c file should have the
"static void" on their own line
Please distinct clearly which variables are only used for muc tubes (as
the muc_connection) and the one only used for 1-1 tubes.
Maybe we should create 2 subclass at some point.
useless double blank lines
priv->xmpp_connection = conn;
I'd make _initalise_connection (self, conn) and move the
assignment/refing to it
seems this function is misnamed and its only purpose is to create the
+void tubes_message_close_received (SalutTubesChannel *self,
the void should be on its own line
DEBUG ("received a tube close message on a non existant tube");
tubes_muc_message_received, tubes_message_received and
tubes_message_close_received should be prefixed with salut_tubes_channel
On a style node, I never understood the point to prefix static method
with _. They are static and don't have the module prefix so I think
that's enough to distinct them from public methods.
please group declarations in the if block
gchar *tube_id_str = g_strdup_printf ("%d", tube_id);
I'd separate the assignment from the declaration
_close = close_node != NULL;
please add ( )
Could be worth to do an early return if (_close) to simplify code
tube_type = gibber_xmpp_node_get_attribute (tube_node, "type");
if (g_str_equal (tube_type, "stream"))
Is it NULL safe? Why not use tp_strdiff as we always do?
DEBUG ("The <iq><tube> does not have a correct type=.");
remove the =. and display the bugged type
iq_tube_request_cb: TpTubeType tube_type = TP_TUBE_TYPE_DBUS;
why this type by default?
DEBUG ("received a tube request, tube_id %d", tube_id);
priv->contact_manager and priv->xmpp_connection_manager are never
*Signal callback for when an Tubes channel is closed. Removes the
what's the point of salut_tubes_manager_handle_tube_request ? It seems
Same question about salut_tubes_manager_handle_si_stream_request. We
don't want to use SI for 1-1 tubes.
- if (bytestream != NULL)
- g_object_set (tube, "bytestream", bytestream, NULL);
I don't understand this change, when is set the bytestream now ?
+ klass->offer_needed = NULL;
it should implement this method
This salut_tube_iface_offer_needed looks very cracky to me. Can't we use
the tube's state to determine if we need to send the offer or not?
+ g_assert_not_reached ();
node = gibber_xmpp_node_add_child (si_node, "stream");
Please remove the assignation after the assertion as it's pointless now
start_stream_direct: early return if salut_contact_manager_get_contact
priv->xmpp_connection_manager is never unreffed
salut_tube_stream_close: what about if
salut_xmpp_connection_manager_request_connection doesn't return DONE ?
salut_tube_stream_close: DEBUG ("ERROR: '%s'", error->message);
make the error message more informative
If the CHANNEL property doesn't make sense anymore, remove it.
Please test if tube-dbus-muc.py, tube-stream-muc.py and
tube-stream-private.py work with your branch.
More information about the Telepathy