[Bug 25493] GTalk-compatible file transfers are not implemented

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=25493





--- Comment #7 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-03-19 11:44:44 PST ---
More comments:

I haven't reviewed the HTTP implementation in detail. It makes me sad that you
need to reimplement HTTP, particularly when Gabble already links against
libsoup! I think it may be time for a feature request against libsoup, asking
for it to implement HTTP over an arbitrary GIOStream...

Design
======

As far as I can tell the relationships are:

* Each file collection has a Jingle session
* Each Jingle session that is for a file collection has one content,
  which is a GabbleJingleShare
* => each file collection has one GabbleJingleShare
* each content shares one or more files or directories, each of which is a
  Jingle component, which ends up making a JingleChannel struct
* each JingleChannel (file) has a pseudoTCP stream (a NiceAgent) over which
  it makes one HTTP request

but ideally it shouldn't have taken me this long to work that out :-)
Using different names as I previously suggested would hopefully make it
clearer what's going on.

I'm not sure what's going on with the current_channel stuff, though - if each
component has its own NiceAgent and pseudo-TCP stream, why do they need to
be multiplexed like this?

You do a Google relay-allocating round trip for each component in the file
collection. Is it possible/desirable to count the components ahead of time, and
do a single batch of relay allocations (like the way we get RTP and RTCP
relays simulataneously for media channels)?

Corner cases
============

transport_disconnected_cb
> +  /* TODO: what about a FT from gtalk, receiving a directory where the size
> +     is just an estimate.. we might be > size, but the FT hasn't completed yet */

What about that? Does GTalk provide a reliable way to tell us "... and
that's all"? Perhaps you could look at libjingle and see how they detect the
end?

If you want to get this merged without addressing this issue, please file
a bug, and reference it in the comment as "FIXME: fd.o #nnnnn: ....".

> -  g_signal_connect (self->priv->listener, "new-connection",
> -    G_CALLBACK (new_connection_cb), self);
> +  gabble_signal_connect_weak (self->priv->listener, "new-connection",
> +    G_CALLBACK (new_connection_cb), G_OBJECT (self));

This looks like a bugfix. Candidate for fast-tracking in?

> +      /* FIXME: if we have access to google relays, should we give priority to
> +         jingle-share instead of SI ? */

Probably yes? If you don't plan to fix this, please file a bug and reference
it as "FIXME: fd.o #nnnnn: ...".

Trivial
=======

> +content_new_channel_cb (GabbleJingleContent *content, const gchar *name,
...
> +      (gpointer *)&relay_data->self);

This violates strict aliasing (so the compiler might mis-optimize it). Put
a gpointer in the struct :-(

> +gabble_ft_manager_channels_created (GabbleFtManager *self, GList *channels)
...
> +      g_hash_table_insert (new_channels, chan, NULL);

This would be wrong if the channels could satisfy a request. I think this
deserves a comment, or just renaming the function to
..._add_incoming_channels.

> +new_jingle_session_cb (GabbleJingleFactory *jf,
...
> +      if (g_list_length (channels) > 0)

This is an inefficient O(n) way to spell "if (channels != NULL)", which would
be O(1).


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