[Telepathy] Review of telepathy-salut.git/tubes

[Guillaume Desmottes]

This is a first review of
http://monkey.collabora.co.uk/telepathy-salut.git_tubes/

tubes.txt
=========

what's the status of 1-1 D-Bus tubes? We should clarify it before
merging.

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.


gibber-bytestream-direct
========================

Maybe bytestream-direct-tcp or bytestream-raw-tcp would be a better
name?

header file: args should be 4 spaces aligned

      case PROP_PROTOCOL:
        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

static void
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 ?

gibber-iq-helper
================

Why remove the if (!NULL) check when disposing? We always check that in
dispose functions.


salut-bytestream-manager.h wasn't removed


salut-direct-bytestream-manager
===============================

  /* guint id -> guint listener_watch
   * When used by stream tubes, the id is the tube_id */
  GHashTable *listeners;

That's a lie, this hashtable contains
SalutDirectBytestreamManagerListener objects.
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
GSList ?
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
as value_destroy_func

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.

listener_io_in_cb
shouldn't it return TRUE to continue listening?

listener_io_in_cb
you should check if the connection is coming from the right contact

salut_direct_bytestream_manager_stop_listen:
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?

salut-tubes-channel
===================

function declarations at the begin of the .c file should have the
"static void" on their own line

struct _SalutTubesChannelPrivate
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.

_initialise_connection
useless double blank lines

priv->xmpp_connection = conn;
g_object_ref (priv->xmpp_connection);
 _initialise_connection (self);
I'd make _initalise_connection (self, conn) and move the
assignment/refing to it

tubes_message_received
seems this function is misnamed and its only purpose is to create the
tube

+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");
s/existant/existent

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.

_send_channel_iq_tube
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

salut-tubes-manager
===================

  _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);
s/tube_id/tube id

priv->contact_manager and priv->xmpp_connection_manager are never
unreffed

*Signal callback for when an Tubes channel is closed. Removes the
references
s/an/a

what's the point of salut_tubes_manager_handle_tube_request ? It seems
called nowhere.

Same question about salut_tubes_manager_handle_si_stream_request. We
don't want to use SI for 1-1 tubes.

tube-dbus
=========

-  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


tube-iface
==========

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?

tube-stream
===========
+      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
returns NULL

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.


Regards,


	G.