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

[bugzilla-daemon at freedesktop.org]

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





--- Comment #10 from Youness Alaoui <youness.alaoui at collabora.co.uk>  2010-03-19 13:57:27 PST ---
(In reply to comment #6)
> After some IRC discussion I've started reviewing the flattened version of
> shared-ft. I don't think implementing the whole lot at once is necessarily the
> best way to have done it, but it's happened now.
> 
> This is not a full review, but some of the changes I'm suggesting may be
> disruptive (naming/layering improvements), so I thought I should put them on
> the bug sooner rather than later.
> 
> Naming
> ======
> 
> Easy to do but somewhat important to make the code understandable:
> 
> I don't like the naming of the "GtalkFtManager" - the name misleads me into
> thinking it's either a ChannelManager, or something that manages GtalkFT
> objects,
> neither of which it actually is (and as a trivial side point, I don't like the
> miscapitalization of GTalk). Perhaps it could mirror the D-Bus API, and hence
> be
> called a GTalkFileCollection? I think that'd make a lot of sense, and avoid the
> confusing code where a variable called gtalk_ft holds a GtalkFtManager.
Yep, I also didn't like it, for me, it was more because of the confusion with
'ChannelManager'. Unfortunately, I'm not good at names :p GtalkFileCollection
is a good idea though! :)

> 
> I'm going to call this a file collection from now on, until/unless someone
> suggests a better name :-)
> 
> We'd conventionally call this GabbleGTalkFileCollection by way of namespacing,
> although I can see that that name is getting a bit unwieldy, so I can
> understand why you omitted the prefix.
Ok, for now, I'll wait until we discuss a better name (IRC),
GTalkFileCollection is good for me, GabbleGTalkFileCollection is a bit too much
imho.

> 
> As noted in comment #5, it's very confusing that the <channel>s from GTalk's
> protocol are called "channels" in the code, given that this is Telepathy code
> where a channel means something different. Could they be called components,
> perhaps? They seem to correspond 1:1 to Jingle components, AFAICS?
Yep, I explained my reasons in the previous comment. Feel free to ping me on
IRC when you'd like to discuss this issue further.

> 
> I was very surprised to see structs called "GabbleChannel" and "JingleChannel"
> within the file collection. GabbleChannel seems to be a thin wrapper around a
> channel object, which is likely to be obsoleted by the structural changes I
> suggest below. If it isn't, I think it should just be replaced by a pointer to
> the channel object, and its boolean attributes should either become methods on
> the channel object, or be incorporated into the methods that are currently
> conditional on them.
yeah, GabbleChannel is an ugly name, but I chose it to differentiate from the,
then existing, JingleChannel struct. And no, its booleans can't be moved into
the GabbleFileTransferChannel because they are specific to the gtalk multiFT as
they help track which FT is 'usable' (accepted but not finished) and which FT
requests the stream to be readable or block reading.

> 
> JingleChannel probably deserves to be a real object with methods and stuff: its
> name is also far too generic, and I'd call it [Gabble]GTalkFileComponent or
> something.

Oh, I love the JingleChannel name, I think it's very specific because it
represents a single jingle channel. The channel is a stream, and the
JingleChannel represents that.


> 
> Structural/design
> =================
> 
> > -  PROP_BYTESTREAM,
> 
> See earlier patch-by-patch review - I think this should remain a (nullable)
> property, and the newly renamed file collection should be another. With the
> layering changes I'm about to suggest, they could both be construct-only, since
> any subsequent change from NULL to non-NULL during the offer process would be
> done internally.
Already fixed :)

> 
> > +      gtalk_ft_manager_terminate (self->priv->gtalk_ft, self);
> > +      /* the terminate could synchronously unref it and set it to NULL */
> > +      if (self->priv->gtalk_ft != NULL)
> > +        {
> > +          g_object_unref (self->priv->gtalk_ft);
> > +          self->priv->gtalk_ft = NULL;
> > +        }
> 
> This is rather terrifying. Isn't there a better way we could do this
> relationship?
Oh there is even more terrifying :

          /* The terminate should call our terminated_cb callback which should
             terminate all channels which should unref us which will unref the
             jingle session */
          self->priv->status = GTALK_FT_STATUS_TERMINATED;
          gabble_jingle_session_terminate (self->priv->jingle,
              TP_CHANNEL_GROUP_CHANGE_REASON_NONE, NULL, NULL);

But that's just how it works when you have synchronous signals/callbacks.
In the code you stated, it's because the terminate would make the underlying
object change state to 'terminated' which would cause the FT channel to call
close_session_and_transport. So it's a classical re-entrance problem that I
fixed that way.
If you have solutions to that kind of stuff (mainly the one I just pasted), let
me know :)

> 
> For a modular design (good layering), 
Thanks
> there should be a concept of objects
> being in an order: objects are allowed to know about lower layers, but not
> about
> higher layers. I think we need to decide between these two stacks:
> 
> * GabbleFtManager > GTalkFileCollection > GabbleFileTransferChannel
> * GabbleFtManager > GabbleFileTransferChannel > GTalkFileCollection
> 
> I initially thought the first one made more sense, but after looking at the
> code a bit more, I think you're right to have the channel ref the file
> collection (which means you're half way to having the second relationship).
> This has the nice side-effect that the GTalkFileCollection could probably
> end up in Wocky one day.
Yes I know, and I didn't want to have the file collection calling methods on
the channel and vice-versa, but there was just no way (imho) to do it because I
can't use signals.. I'll explain it below..

> 
> As far as I can tell, the calls between the objects are:
> 
> ChannelManager calling into file collection:
> 
> * new_from_session() (when receiving)
> * get_channels() (when receiving)
> 
> Channel calling into file collection:
> 
> * new() then initiate() (when offering)
> * block_reading()
> * accept()
> * send_data()
> * completed()
> * terminate()
> 
> File collection calling into channel:
> 
> * new() (in new_from_session()) [1]
> * set_gtalk_ft() during add_channel() [2]
> * set_gtalk_ft_state() [3]
> * gtalk_ft_write_blocked() [4]
> * gtalk_ft_data_received() [5]
> 
> My suggestion for the file collection's API would go something like this:
> 
> When created to receive files, all its components should be blocked. Most of
> the logic of new_from_session() should move into the GabbleFtManager, so the
> GabbleFtManager would take care of creating a Telepathy channel for each
> component, and handing the file collection and component number to the
> channel as a construct-time property (much like the current handling of
> bytestreams). This eliminates [1] and [2].
Ok, this makes sense.. [2] is already removed, so only removing [1] would be
necessary.. and I'd like to get rid of that get_channels method.
I can fix this by moving the manifest looping/channel creation into the
ChannelManager, It should make it cleaner.


> 
> The channel would unblock the relevant component on the file collection,
> and off we go: subsequently, the channel would "drive" the file collection, a
> lot like it now does with bytestreams. [3], [4] and [5] would have to become
> signals or something: perhaps they could even have a boolean return value, so
> the file collection can block itself automatically if nobody's reading yet?
Well that's the thing, that's the whole issue of why I had to do it this way..
it just doesn't work, you can't use signals, there is no use case where "nobody
is reading yet". The thing is that you could have 10 FT Channels, but you'll
only have one file collection, and even if they are all accepted, you can only
have one file being transferred at a time because it's using the HTTP
server/client to transfer files and you can't do multiple transfers on one
socket (one jingle channel). We won't receive any data until we send a request,
and we won't send a request until we elect a channel as the currently selected
one.
We can't use signals because if we send a signal, then all channels would be
listening to it, if the file collection (FC) changes its status, it doesn't
want all channels to know about it, it only needs specific channels to know
about it.. example :
3 FTs, you accept one, it causes the FC to accept the jingle session which
would then make it change its status to 'accepted' then 'open'.. if all FTs
listen to the state changes, then the 2 PENDING channels will see that it's not
"accepted"/"open" and will change their status accordingly, when the user
accepts the second FT, it will fail because "Can't call AcceptFile on a channel
that is not in the PENDING state".. problem is, it went to state ACCEPTED but
the UI never get the socket to connect to the channel's pipe.
same with data.. we receive data, we don't want to have all channels receive
it, we want only the channel related to the file we're receiving to receive its
data...
Also, if you noticed, I keep all accepted channels in the state ACCEPTED, and I
only have one channel in the state OPEN at a time, it is to avoid many possible
issues and race conditions that I did it like that.. so signals are a NO GO.

A possibility is to specify which channel is to be affected as the first
argument of every signal, but I find it even uglier than the current solution.


> 
> Another good way to handle this might be to take the JingleChannel struct
> from the file collection, and either merge its functionality into
> GabbleJingleShare (if possible), or promote it to an object in its own
> right, analogous to the bytestream objects we already have. [3], [4] and [5]
> would then be signals emitted from this object, rather than from the
> file collection?
Same problem.. yes, I could extract the JingleChannel, and I actually wanted to
do this from the very beginning and I had a lot of trouble finding which object
(jingle-share, jingle-content, jingle-transport-google, channel manager or the
FT channel) should do this whole functionality (libnice integration,
creation/managing of the jingle channel/stream).
And I found that I could only do it inside the FT channel (there was no
GtalkFTManager at that time). I can't remember why exactly, but there was
always something that was needed that the other objects didn't have access to.
But yes, imho, it should be in jingle-transport-google.c


> 
> Not obvious
> ===========
> 
> These might be explained better in the commit log, I haven't checked yet
> (I haven't been reading commit messages during this whole-branch review).
> 
> > -  if (self->priv->state != TP_FILE_TRANSFER_STATE_COMPLETED)
> > +  if (self->priv->state != TP_FILE_TRANSFER_STATE_COMPLETED &&
> > +      self->priv->state != TP_FILE_TRANSFER_STATE_CANCELLED)
> 
> I suspect that the guard against cancelling an already-cancelled FT is good,
> but is this a fix for a pre-existing bug, or what?
Would help to know where that change is exactly :) 
But I suppose it's in gabble_file_transfer_channel_close. In which case the
reason is simple, I was testing when the remote cancels the FT, the channel
goes into state CANCELLED with reason REMOTE_STOPPED, then it closes itself and
sends another signal CANCELLED with reason LOCAL_STOPPED.
I did it to preserve the reason of the cancellation (IIRC)

> 
> > -      if (self->priv->transport)
> > +      if (self->priv->transport &&
> > +          (self->priv->bytestream || self->priv->gtalk_ft))
> 
> Why did this logic change? Is it possible for channel_open() to be called
> with neither a bytestream nor a GTalk FT?
Well, the only reason this could happen is during a full moon, so better be
safe than sorry....
Actually, it was if (transport), I changed it to if (transport && bytestream)
because the jingle case was handled already somewhere else, then I realized
(after the refactor) that it might still be needed so I had if (transport &&
bytestream)... if (transport && bytestream) ..., then I cleaned it up the way
you see it above..
Good job spotting this! I'll fix it now.



> 
> > +gabble_file_transfer_channel_set_bytestream (GabbleFileTransferChannel *self,
> ...
> >    if (bytestream == NULL)
> > -    return;
> > +    return FALSE;
> 
> Not your fault (pre-existing code), but why isn't this a g_return_if_fail?
> If it's deliberate that a NULL bytestream is silently ignored, I'd at least
> expect a comment above the function.
The only reason it can be NULL (before gtalk_ft) is when you want to *offer*
the FT, in which case it starts with its default value of NULL, then offer_file
is called and negotiate_bytestream_cb sets the bytestream.

> 
> > +  if (self->priv->bytestream || self->priv->gtalk_ft)
> > +    return FALSE;
> 
> Similarly, why would it not be a programming error to assign more than one?
> Exactly the same applies to ..._set_gtalk_ft.
the FT channel either does SI file transfers or jingle file transfers, it can't
use multiple protocols at the same time...

> 
> > -  self->priv->remote_accepted = TRUE;
> 
> Why is it OK that bytestream_negotiate_cb() no longer does this?
This was explained in the relevant commit : 
http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=3817195359b622b0f1e35ee3d2d041650465e33d

> 
> Minor stuff
> ===========
> 
> > +PKG_CHECK_MODULES(NICE, nice >= 0.0.10.1)
> 
> Should be 0.0.11 - we don't want to depend on any unreleased module
Yep, but couldn't test until 0.0.11 was released (yesterday!). Will fix,
thanks, I had forgotten about it to be honest :)

> 
> > -close_bytestrean_and_transport (GabbleFileTransferChannel *self)
> > +close_session_and_transport (GabbleFileTransferChannel *self)
> 
> gabble_file_transfer_channel_close_transports(), perhaps?
it's private/static so there's no gabble_file_transfer_channel_ prefix, and
here, 'transport' refers to the gibber transport conncting gabble to the UI,
and 'session' is the bytestream or the jingle session, so I think
close_session_and_transport is a good name for it.

> 
> For future reference
> ====================
> 
> (Things that don't need fixing here, but should be considered in future)
> 
> > +<node name="/Channel_Type_FileTransfer_Future"
> 
> This should be /Channel_Type_File_Transfer_Future really, but it doesn't matter
> much since this interface is temporary. In the Telepathy code generation stuff,
> the node name is (ab)used to name auto-generated identifiers - this mistake is
> why you have GABBLE_IFACE_CHANNEL_TYPE_FILETRANSFER_FUTURE rather than
> GABBLE_IFACE_CHANNEL_TYPE_FILE_TRANSFER_FUTURE, for instance.
haha, I put it with File_Transfer first, then I checked and found the interface
is Channel.Type.FileTransfer so I renamed it, lol.

> 
> Please also replace tabs with an appropriate number of spaces (setting your
> editor to highlight tabs may help). Again, it doesn't matter much here, since
> we'll fix it when incorporating this functionality in the spec.
ah, it's set not to put tabs at all, but I guess since it's .xml and not .c,
those settings were not loaded... 

> 
> > +static void transferred_chunk (GabbleFileTransferChannel *self, guint64 count);
> 
> I generally prefer to avoid forward declarations and just put the static method
> above the first call to it. (It's hard to do that for GInterface initialization
> functions, since they need to be called in the G_DEFINE_TYPE_WITH_CODE, yet
> reference method implementations, but it's easy to do for most helper
> functions.)
> 
> I also somewhat prefer private functions to be properly namespaced
> (gabble_file_transfer_channel_foo(), or a somewhat abbreviated version like
> ft_channel_foo()) even though they're static - this results in a bit more
> typing, but better debug messages (the DEBUG macro automatically incorporates
> the function name).
> 
> If the function is conceptually a method on a named private data structure
> (like your JingleChannel and GabbleChannel structs, for instance), it's nice to
> name it accordingly (pending_stun_server_free() in jingle-factory.c is a simple
> example).
> 
I see, I'll try to remember that, but I usually do "no prefix for
static/private functions" and generally the debug message give enough info to
know which function from which file we're talking about.. anyways.

Thanks for this initial review!


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