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

[bugzilla-daemon at freedesktop.org]

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





--- Comment #11 from Youness Alaoui <youness.alaoui at collabora.co.uk>  2010-03-19 14:24:58 PST ---
(In reply to comment #7)
> More comments:
== More fun :)

> 
> 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...
Yes, I know it sucks, but it was discussed on IRC, can't remember if you were
there..
I spoke also with zeeshan and he said he would be working on doing just that
(he probably wants it for doing http over UDP for his upnp stuff, can't
remember).. but in the meantime, I reimplemented a very small subset of HTTP.
It's really not complicated and since it will work against gabble/libjingle, it
should be safe enough that we know already what kind of messages to expect.

> 
> 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.
hehe, unfortnuately, there's one error in the design. There's only one
JingleChannel, and the files are queued so you only transfer one at a time...
you have one jingle channel which represents once NiceAgent with one PseudoTcp
stream (so one HTTP client or server), and the files are being sent/received
one at a time through that single stream.
There could be a second jingle channel which is usually used by libjingle in
order to transfer image previews even prior to the user clicking 'accept', but
we don't implement that.

> 
> 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?
See above :)

> 
> 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)?
We actually do it once since we only have one component/jingle channel. So it's
fine :)


p.s: if you're wondering why not do one jingle channel per file transfer, I'd
say it's because libjingle doesn't do it, so we don't need to do it, having
more file being transferred at the same time would slow them all, and we'd get
more packet drops in our UDP stream, etc.. there's no need for doing them in
parallel (also, gtalk, as well as a future FileCollection-aware UI should show
them all as a batch and show a single progressbar for all the files, so it
doesn't matter if they're done in parallel or if they get queued).

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

no, that's ok, we already know the end since it's HTTP, it's transferred with
chunked content encoding, so we know when the last chunk is received. What I
meant but that is :
if the UI disconnects from gabble before the file has finished transferring, we
go into 'canceled' state with LOCAL_ERROR reason, and it does that by checking
if transferred_bytes >= size. But when transferring a directory, the size is an
approximation, and it's lower than the real file size, so it might fail to know
if the disconnection happened before the file finished transferring... So I'd
have to keep track somehow of when the file is completed... 
> 
> 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: ....".
The /*TODO*/ lines are stuff I need to do before the merge, the /*FIXME*/ lines
are stuff that would need to be fixed at some point to make the code
clean/robust, but that corner case would probably never happen in real life.
So I'll try to fix it next week, don't worry.

> 
> > -  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: ...".
Don't plan to fix it now until it's discussed what to do with the SI vs.
jingle-share decision.


> 
> 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 :-(
I would still need to cast 'self' into the relay_data structure, so it's all
the same. as I said in previous comment, I don't think 'gpointer' breaks
strict-aliasing. + there are no warnings..

> 
> > +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.
Added a comment :
      /* The channels can't satisfy a request because this will always be
called
         when we receive an incoming jingle-share session */
I could also allow for specifying the Requests (that's what I planned but..)
but I don't really like the idea of 'two lists, same length, map one to one
depending on the index in the list' kind of algo..

> 
> > +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).
> 
You're right indeed. Fixed. Thanks.


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