0.8.7-1 Can't extabilish file transfer TO and FROM google talk windows clients
That's because they're not implemented; retitling.
Created attachment 33172 [details] [review] gtalk-ft branch collapsed patch I've been working on this, and it should now be implemented. Code is pending review/fixes. You will need the gtalk-ft telepathy-gabble branch as well as the reliable branch of libnice : http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=shortlog;h=refs/heads/gtalk-ft http://git.collabora.co.uk/?p=user/kakaroto/libnice.git;a=shortlog;h=refs/heads/reliable Attaching a collapsed patch of telepathy-gabble.
Having the patch tag is advantageous to get your code reviewed; so is having the telepathy-bugs list in cc or as the QA contact.
I've filed Bug #27164 for the libnice part, which I'm not qualified to review. Regression tests appear to be added in Bug #27162.
I don't have a good overview of this branch and am currently reviewing patch-by-patch: I'll have a look at the overall branch (i.e. collapsed patch) later. Reviewed up to, but not including, <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=a74ef1df3e8b686ad4a959e8e62a6a6070d68a53>: http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=0451d5847fe2b > fix typo bytestrean/bytestream This could usefully be on a trivia branch that's merged ahead of the feature work. http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=4afc4f205175b8: > + GabbleJingleShare *trans = GABBLE_JINGLE_SHARE (object); trans is an odd name for a thing that isn't a transport, and in any case I think you mean "self" :-P > + /* This property is here only because jingle-session sets the media-type > + when constructing the object.. */ > + g_object_class_install_property (object_class, PROP_MEDIA_TYPE, > + g_param_spec_uint ("media-type", "media type", I thought the whole concept of media types was part of Jingle RTP, rather than the layer of Jingle at which file transfers appear? If I'm right, then file transfers should have media type JINGLE_MEDIA_TYPE_NONE? > + //GabbleJingleShare *self = GABBLE_JINGLE_SHARE (content); > + //GabbleJingleSharePrivate *priv = self->priv; > + //JingleDialect dialect = gabble_jingle_session_get_dialect (content->session); No commented-out cruft in commits, please (also, comments should be /* */ rather than // in C code). > +const gchar *gabble_jingle_share_parse (GabbleJingleShare *sess, > + LmMessage *message, GError **error); Doesn't actually seem to exist? > - PROP_BYTESTREAM, I don't like the removal of the bytestream property from the FT channel in favour of bolting one on afterwards: I think I'd prefer it if the bytestream and the Jingle session were both properties, with one or neither supplied during construction. > + if (self->priv->bytestream || self->priv->jingle) (...->bytestream != NULL || ...->jingle != NULL), please, and shouldn't this be a g_return_if_fail? > + if (resource) != NULL > + g_object_set (jingle, "dialect", JINGLE_DIALECT_GTALK4, NULL); We always put one property/value pair per line in g_object_set/g_object_new: g_object_set (jingle, "dialect", J_D_G, NULL); > + g_assert (self->priv->bytestream == NULL && self->priv->jingle == NULL); g_assert (a && b) is typically better written as two assertions: you get a better failure message that way. Also, shouldn't this be a g_return_if_fail? > + self->priv->handle); > + if (presence == NULL) Blank lines around blocks (if/while/etc.), please > + if (error != NULL) > + g_set_error (error, TP_ERRORS, TP_ERROR_OFFLINE, > + "can't find contact's presence"); No need for the guard, g_set_error copes with NULL as a first argument (that's the whole point of using it instead of "*error = g_error_new ()") > + if (self->priv->bytestream) { That's not how we indent. > + size = gabble_jingle_share_get_filesize (c); I'd prefer file_size or just size. http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=2de68bb15afc8d1 > Remove checking for dialect in google transport What does this have to do with anything? The long commit message should explain why this change is desirable. http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=aef01b07e68f6 This patch seems to remove the special handling of the components named video_rtp, video_rtcp, rtp and rtcp. Is that special handling duplicated elsewhere (in which case explain in the long commit message), or have you just introduced a regression? (Reading on, it seems that you did introduce a regression, which you fixed in the following patch. Please don't do that, where possible - aim for each patch to be valid in isolation.) > + GHashTable *channels; /* Google transport 'channels', not TP channels */ Please comment the contents and their ownership: /* Google transport 'channels', not Tp channels * g_strdup'd component name => GINT_TO_POINTER (component ID) */ ... and why isn't it called components or component_ids, which would remove the confusion with Telepathy channels? > + if (g_hash_table_lookup_extended (priv->channels, name, > + NULL, NULL) == FALSE) Boolean results are the one case where we don't do an explicit comparison: please use if (!g_h_l_e (...)) > + GList **all_candidates = NULL; At the very least, this deserves a comment. By inspection, it appears to be an array (of length @components) of GList of JingleCandidate. A GPtrArray, or indeed a GHashTable from component number to GList, might well be a clearer way to do this. > + guint idx; Please avoid uncommon abbreviations. I'd just call it "i", tbh. > + for (idx = 0; idx < components; idx++) { That's not how we indent. I'm going to stop bothering to mention occurrences of this now; consider them all to have been noted :-P > + GList **cands = NULL; I think this would be clearer if it just used i, like so: > for (i = 0; i < components; i++) > { > JingleCandidate *c2 = all_candidates[i]->data; > > if (c->component == c2->component) > break; > } > > if (i >= components) > { > /* not found */ > all_candidates = g_renew (GList *, all_candidates, ++components); > i = components - 1; > } > > all_candidates[i] = g_list_prepend (all_candidates[i], c); Indeed, I remain unconvinced an extending array of GList is the right data structure here. (This does get somewhat better in a later patch, when you change it into a GList of GList.) In fact, you seem to be mapping in an elaborate way between component names and lists of candidates for the component, going via integer component numbers. Accordingly, wouldn't it be easier to just have a map from component name to list of candidates? http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=9bbc9fb9562a Commit message is far too long; we like linebreaks. > + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &c)) This violates strict aliasing. Declare c with type gpointer; its real type will be obvious from context. > jingle_transport_google_set_component_name ( > - GabbleJingleTransportGoogle *transport, gchar *name, gint component_id) > + GabbleJingleTransportGoogle *transport, > + const gchar *name, gint component_id) What's this doing here? It should have been a separate patch "fix const-correctness of jingle_transport_google_set_component_name", which in turn should have been squashed into the commit that introduced that function. http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=c66e8cb4b2fb > +static gchar * > +generate_temp_url (void) Couldn't you just use gabble_generate_id()? > + GList *manifest; Should have a comment to say what it's a list of, and who owns the items > + m->image_width = atoi (width); atoi is locale-sensitive, so that's not what you want. g_ascii_strtoull would be more appropriate. http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=9806c8a5bebc > properly report the direction as 'initiator' for the file transfers Hard-coding special treatment in GabbleJingleContent doesn't seem right. Shouldn't this be some sort of virtual method? http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=caa2d524faa97 > +gen_manifest (GabbleJingleShare *self) ensure_manifest(), perhaps? > + GabbleJingleShareManifest *m = g_slice_new0 (GabbleJingleShareManifest); > + m->name = g_strdup (self->priv->filename); Blank line between declarations and statements, please > - _gabble_jingle_content_set_media_ready (GABBLE_JINGLE_CONTENT (obj)); > + _gabble_jingle_content_set_media_ready (content); This has nothing to do with the purpose of this commit, and should have been squashed into the commit that introduced this line in the first place > +GList *gabble_jingle_share_get_manifest (GabbleJingleShare *self) Newline after GList *, please http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=87453d951a7c > do not create a new manifest if we have no filename Obvious candidate for squashing into previous commit http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=fba2d93160f9d > + NULL, NULL) == TRUE) No explicit comparison for booleans, please - comparing against TRUE is dangerous, because ((gboolean) 42) is a true value which is neither TRUE nor FALSE http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=354bc1deca339 > - gabble_marshal_VOID__STRING_UINT, > + gabble_marshal_VOID__STRING_INT, This part isn't related to the rest of the commit, and would have been better squashed into the commit that introduced this signal > + priv->last_channel_component_id + 1) == FALSE) Same complaint as previously > +new_channel (GabbleJingleContent *c, const gchar *name) If these constructs are isomorphic to Jingle components, I suggest calling them components, even if Google call them channels > +static guint > +new_channel (GabbleJingleContent *c, const gchar *name) Is the component ID signed or unsigned? Please make your mind up. Likewise on gabble_jingle_content_create_channel. > - DEBUG ("changing senders from %s to %s", produce_senders (priv->senders), senders); > + DEBUG ("changing senders from %s to %s", produce_senders (priv->senders), > + senders); Secretly not actually relevant to this patch?
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. 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. 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? 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. 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. 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. > + 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? For a modular design (good layering), 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. 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]. 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? 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? 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? > - 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? > +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. > + 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. > - self->priv->remote_accepted = TRUE; Why is it OK that bytestream_negotiate_cb() no longer does this? 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 > -close_bytestrean_and_transport (GabbleFileTransferChannel *self) > +close_session_and_transport (GabbleFileTransferChannel *self) gabble_file_transfer_channel_close_transports(), perhaps? 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. 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. > +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).
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).
I'll try to get the HTTP implementation etc. reviewed next week sometime...
(In reply to comment #5) > I don't have a good overview of this branch and am currently reviewing > patch-by-patch: I'll have a look at the overall branch (i.e. collapsed patch) > later. > > Reviewed up to, but not including, > <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=a74ef1df3e8b686ad4a959e8e62a6a6070d68a53>: > Thanks for reviewing! > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=0451d5847fe2b > > fix typo bytestrean/bytestream > > This could usefully be on a trivia branch that's merged ahead of the feature > work. If you want I could cherry pick it, but I don't think it's really necessary atm, it's not like it's a bugfix or anything... > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=4afc4f205175b8: > > + GabbleJingleShare *trans = GABBLE_JINGLE_SHARE (object); > > trans is an odd name for a thing that isn't a transport, and in any case I > think you mean "self" :-P Yeah, copy paste errors probably :) I checked, and it seems it was already fixed in a later commit. > > > + /* This property is here only because jingle-session sets the media-type > > + when constructing the object.. */ > > + g_object_class_install_property (object_class, PROP_MEDIA_TYPE, > > + g_param_spec_uint ("media-type", "media type", > > I thought the whole concept of media types was part of Jingle RTP, rather than > the layer of Jingle at which file transfers appear? If I'm right, then file > transfers should have media type JINGLE_MEDIA_TYPE_NONE? Well, the way I see it is that you have a 'media type file' for file transfers (because, that's what it is, a ICE/UDP stream, in which you send a file). But the reason this is there is explained in the comment. The jingle-session creates the object with MEDIA_TYPE=MEDIA_TYPE_NONE, then it changes it depending on the type of content in the jingle-session. There's just no avoiding it with the way the jingle code works, at least, not without refactoring it or adding special use cases, which I didn't want to do. Simple solution, with an explanation comment seemed safer. > > > + //GabbleJingleShare *self = GABBLE_JINGLE_SHARE (content); > > + //GabbleJingleSharePrivate *priv = self->priv; > > + //JingleDialect dialect = gabble_jingle_session_get_dialect (content->session); > > No commented-out cruft in commits, please (also, comments should be /* */ > rather than // in C code). Yeah, those were temporary, they got removed/fixed later. But yeah, I'm also surprised I didn't see them when I did the 'git add -p'. > > > +const gchar *gabble_jingle_share_parse (GabbleJingleShare *sess, > > + LmMessage *message, GError **error); > > Doesn't actually seem to exist? You're right. Removed. > > > - PROP_BYTESTREAM, > > I don't like the removal of the bytestream property from the FT channel in > favour of bolting one on afterwards: I think I'd prefer it if the bytestream > and the Jingle session were both properties, with one or neither supplied > during construction. Humm, either way is fine I think, it's just a matter of choice. But I agree that having them both as properties would probably be nicer. I'll fix that. > > > + if (self->priv->bytestream || self->priv->jingle) > > (...->bytestream != NULL || ...->jingle != NULL), please, and shouldn't this be > a g_return_if_fail? > > > + if (resource) > > != NULL ok, sorry, I didn't know about this convention, I usually put the == NULL, but not the != NULL. I'll fix it. About g_return_if_fail, I simply forgot it exists. > > > + g_object_set (jingle, "dialect", JINGLE_DIALECT_GTALK4, NULL); > > We always put one property/value pair per line in g_object_set/g_object_new: > > g_object_set (jingle, > "dialect", J_D_G, > NULL); Didn't know it was a convention. Now fixed. Thanks. > > > + g_assert (self->priv->bytestream == NULL && self->priv->jingle == NULL); > > g_assert (a && b) is typically better written as two assertions: you get a > better failure message that way. Also, shouldn't this be a g_return_if_fail? Very much true. Thanks, fixed. > > > + self->priv->handle); > > + if (presence == NULL) > > Blank lines around blocks (if/while/etc.), please > > > + if (error != NULL) > > + g_set_error (error, TP_ERRORS, TP_ERROR_OFFLINE, > > + "can't find contact's presence"); > > No need for the guard, g_set_error copes with NULL as a first argument (that's > the whole point of using it instead of "*error = g_error_new ()") > Don't know, that was copy/pasted code. Anyways, I won't "fix" this because I prefer to have the guard than not have it. > > + if (self->priv->bytestream) { > > That's not how we indent. Yes, they were all fixed later on in another commit, I greped for ')' and '{' and there are none (from my code anyways). > > > + size = gabble_jingle_share_get_filesize (c); > > I'd prefer file_size or just size. That method disappeared after the refactor.. > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=2de68bb15afc8d1 > > Remove checking for dialect in google transport > > What does this have to do with anything? The long commit message should explain > why this change is desirable. Oh, it's simple, it checks to see if the dialect is 'google' in the 'google transport'.. it was not necessary and made no sense because the google-transport is already only used when we are in google dialect. Sorry, I thought it would be obvious. > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=aef01b07e68f6 > > This patch seems to remove the special handling of the components named > video_rtp, video_rtcp, rtp and rtcp. Is that special handling duplicated > elsewhere (in which case explain in the long commit message), or have you just > introduced a regression? > > (Reading on, it seems that you did introduce a regression, which you fixed in > the following patch. Please don't do that, where possible - aim for each patch > to be valid in isolation.) I wanted to make small patches, not necessary all fully working, that's why I separated these two changes into two separate commits, to make it easier to read/review/understand. I usually don't aim for each patch to be 'valid' but rather to be 'isolated set of related changes'. > > > + GHashTable *channels; /* Google transport 'channels', not TP channels */ > > Please comment the contents and their ownership: > > /* Google transport 'channels', not Tp channels > * g_strdup'd component name => GINT_TO_POINTER (component ID) */ > > ... and why isn't it called components or component_ids, which would remove the > confusion with Telepathy channels? ok, I'll comment my hash table/list contents. And it's 'channels' because that's the jingle language. They are called 'channels'. Yes, it clashes with telepathy's naming, but they are still a 'jingle channel', and I don't think it's wise to start renaming them to something else, otherwise it will add confusion and will make it harder later on. And they are not 'components', it is just how I decided to store the information.. since jingle doesn't hold the 'channel name', it only used component ids (1 for rtp, 2 for rtcp), I decided to assign each jingle channel a unique id (irrelevant to the protocol) and use that as component_id. But technically, it's a jingle channel, where locally I assign to it a unique id that I use as a component_id, but it's not one since each 'channel' (each 'component id') will have its own NiceAgent, it won't be an additional component on the same ICE Agent. it's a bit complicated, and maybe I should document it somewhere to make it clearer... If you're still confused about this, let me know (I know I'm bad at explaining stuff) > > > + if (g_hash_table_lookup_extended (priv->channels, name, > > + NULL, NULL) == FALSE) > > Boolean results are the one case where we don't do an explicit comparison: > please use if (!g_h_l_e (...)) I'd have to disagree, I always *hate* it when I see if (!bool) because it makes it so much harder to read/understand the code.. the '!' is so subtle, you don't always spot it, and I just find it hard... Anyways, I don't like this convention, but if it's telepathy convention, I got no choice, so I've fixed this for you. > > > + GList **all_candidates = NULL; > > At the very least, this deserves a comment. By inspection, it appears to be an > array (of length @components) of GList of JingleCandidate. A GPtrArray, or > indeed a GHashTable from component number to GList, might well be a clearer way > to do this. Yep, that part was ugly, iirc, I asked sjoerd to review that function so we can discuss a better method of doing it, and it got refactored later on. > > > + guint idx; > > Please avoid uncommon abbreviations. I'd just call it "i", tbh. 'idx' is usually for 'index' and it's a common abbreviation. it's also used in the media-channel code.. > > > + for (idx = 0; idx < components; idx++) { > > That's not how we indent. I'm going to stop bothering to mention occurrences of > this now; consider them all to have been noted :-P Yep, I know, needed a little getting used to, but like I said before, they were all fixed already in later commits. > > > + GList **cands = NULL; > > I think this would be clearer if it just used i, like so: > > > for (i = 0; i < components; i++) > > { > > JingleCandidate *c2 = all_candidates[i]->data; > > > > if (c->component == c2->component) > > break; > > } > > > > if (i >= components) > > { > > /* not found */ > > all_candidates = g_renew (GList *, all_candidates, ++components); > > i = components - 1; > > } > > > > all_candidates[i] = g_list_prepend (all_candidates[i], c); > > Indeed, I remain unconvinced an extending array of GList is the right data > structure here. (This does get somewhat better in a later patch, when you > change it into a GList of GList.) > > In fact, you seem to be mapping in an elaborate way between component names and > lists of candidates for the component, going via integer component numbers. > Accordingly, wouldn't it be easier to just have a map from component name to > list of candidates? Yes, it does get better later like I explained above. I will ignore the current comments since they seem out of place with the code as it is now, so if you can review the function in its current state and maybe provide an insight on how to do it better. > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=9bbc9fb9562a > > Commit message is far too long; we like linebreaks. Yep, learned my lesson already :) > > > + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &c)) > > This violates strict aliasing. Declare c with type gpointer; its real type will > be obvious from context. Humm, are you sure there will be strict aliasing errors with 'gpointer'? I'm not getting any warning at compilation. Also, if I declare it as gpointer, then I'd still need to cast it into GabbleJingleContent in order to call gabble_jingle_content_parse_info > > > jingle_transport_google_set_component_name ( > > - GabbleJingleTransportGoogle *transport, gchar *name, gint component_id) > > + GabbleJingleTransportGoogle *transport, > > + const gchar *name, gint component_id) > > What's this doing here? It should have been a separate patch "fix > const-correctness of jingle_transport_google_set_component_name", which in turn > should have been squashed into the commit that introduced that function. I'm trying to make small commits all the time, but at some point, there's a limit, I don't think this is an issue to have it slip in an unrelated commit ;p > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=c66e8cb4b2fb > > +static gchar * > > +generate_temp_url (void) > > Couldn't you just use gabble_generate_id()? didn't know it exists... but after looking at it, it seems to generate in a format or "%lx.%lx/%lx/%lx" or uuid (I don't know the format of uuid), but since I'm building a path here, I'm not sure it's a great idea to have '/' in there.. i'd prefer just using the simple code I have. > > > + GList *manifest; > > Should have a comment to say what it's a list of, and who owns the items it was refactored, it's better now. > > > + m->image_width = atoi (width); > > atoi is locale-sensitive, so that's not what you want. g_ascii_strtoull would > be more appropriate. oups, probably some copy/paste from libjingle. Fixed, thanks! > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=9806c8a5bebc > > properly report the direction as 'initiator' for the file transfers > > Hard-coding special treatment in GabbleJingleContent doesn't seem right. > Shouldn't this be some sort of virtual method? it doesn't seem right indeed, but that's the best I could do. That whole jingle code has 'if media type == video' and 'if media type == audio' all over the place, and there are many thing that are very audio/video specific, so I just pitched in use cases for non audio/video jingle. Do you suggest I change this before merging? in that case, how do you suggest to fix it ? > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=caa2d524faa97 > > +gen_manifest (GabbleJingleShare *self) > > ensure_manifest(), perhaps? yep, fixed. > > > + GabbleJingleShareManifest *m = g_slice_new0 (GabbleJingleShareManifest); > > + m->name = g_strdup (self->priv->filename); > > Blank line between declarations and statements, please fixed them all > > > - _gabble_jingle_content_set_media_ready (GABBLE_JINGLE_CONTENT (obj)); > > + _gabble_jingle_content_set_media_ready (content); > > This has nothing to do with the purpose of this commit, and should have been > squashed into the commit that introduced this line in the first place you are too verbose.. :p > > > +GList *gabble_jingle_share_get_manifest (GabbleJingleShare *self) > > Newline after GList *, please fixed them all. > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=87453d951a7c > > do not create a new manifest if we have no filename > > Obvious candidate for squashing into previous commit Nope, they were separated on purpose.. since the previous commit just moved that chunk of code into its own function, if I had squashed this too (or git added it) then this patch/fix would have been lost in the change.. I don't like reviewing a commit where code is moved from one place to another, and you have to manually do the diff to see if it was moved or 'moved and modified'. > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=fba2d93160f9d > > + NULL, NULL) == TRUE) > > No explicit comparison for booleans, please - comparing against TRUE is > dangerous, because ((gboolean) 42) is a true value which is neither TRUE nor > FALSE as stated above, I disagree, and they are all fixed to suit your taste now. But as an FYI, casting '42' as a gboolean is bad, I would rather do : gboolean b = x > 0? TRUE: FALSE; And I got into the habit of checking for == TRUE/FALSE because we were forced to do that in my previous job, because on some systems (can't remember if it was AIX or MPE/x) TRUE is 0xFFFFFFFF, everything else is FALSE... (which is also why we do '#define TRUE (1 == 1)', and not '#define TRUE 1') > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=354bc1deca339 > > - gabble_marshal_VOID__STRING_UINT, > > + gabble_marshal_VOID__STRING_INT, > > This part isn't related to the rest of the commit, and would have been better > squashed into the commit that introduced this signal Nope, because at that time UINT made sense and now, in this commit in which I actually call g_signal_emit, it made sense to make it a INT. > > > + priv->last_channel_component_id + 1) == FALSE) > > Same complaint as previously same response as previously :) > > > +new_channel (GabbleJingleContent *c, const gchar *name) > > If these constructs are isomorphic to Jingle components, I suggest calling them > components, even if Google call them channels as explained above, they are not the same as a component, it's just a 'hack' that I transform the 'name' into a 'int' and I use the existing (and previously unused in our case) component_id variable to store it. I could also just create a 'gchar *channel' variable instead in the JingleCandidate.. Anyways, this should be discussed over IRC if you don't agree. > > > +static guint > > +new_channel (GabbleJingleContent *c, const gchar *name) > > Is the component ID signed or unsigned? Please make your mind up. Likewise on > gabble_jingle_content_create_channel. Humm.. I was hesitant at the start, then I had made up my mind as a guint, I just checked it's unsigned pretty much everyone, but I missed some spots. Fixed now. > > > - DEBUG ("changing senders from %s to %s", produce_senders (priv->senders), senders); > > + DEBUG ("changing senders from %s to %s", produce_senders (priv->senders), > > + senders); > > Secretly not actually relevant to this patch? > not ninja enough :(
(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!
(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.
(In reply to comment #8) > I'll try to get the HTTP implementation etc. reviewed next week sometime... > I hope the only thing remaining now is to review the HTTP implementation! :D So, iirc, the main points remaining are : - renaming gtalk_ft_manager (find a new proper name then do it) - discuss whether or not jingle channels need to be renamed to something else.. - discuss the design of the file collection calling methods on the channel vs. signals. And there are other smaller things on which we disagreed.. Thanks a lot for this review. I appreciate it! ps.: I'm really not sure it comes from my code, but I have a lot of core dumps that seem to get generated when I do test the gtalk ft stuff, but the stacktrace are almost always the same, and they have absolutely nothing to do with my code.. try 'make check' a few times on the jingle-share tests to reproduce... if you have any idea... thx
Based on discussion on IRC, a diagram of the object relationships is (at least temporarily) available here: http://people.collabora.co.uk/~smcv/20100322_001.jpg
So, after a long discussion over IRC, here is a summary of the tasks that I need to do to fix the various issues discussed previously : - renaming GtalkFTManager into GTalkFileCollection - rename JingleChannel (and jingle channel references) into ShareChannel - move the manifest parsing into the GabbleFileTransferChannelManager and remove the call to 'gabble_file_transfer_channel_new' from the GTalkFileCollection - rename gabble_file_transfer_channel_set_gtalk_ft_state into gabble_file_transfer_channel_gtalk_ft_state_changed to make it clear it's a 'signal-like' API. - add comments explaining the design and layering violation, as well as the protocol and object associations - remove GabbleChannel and use the channel's 'ACCEPTED' state for 'usable' and use a GHashTable for the 'readable' fields of GabbleChannel. Here is a list of the other issues discussed previously that were not addressed yet or to which I didn't previously reply with 'fixed' : - Do you still want me to cherry-pick the bytestrean/bytestream typo fix ? - You still want me to remove/refactor the 'media-type' property ? - 'blank line around blocks' -> forgot to say 'fixed' - remove the check for error != NULL around g_set_error -> after discussion with Will, I've fixed it. - asked to avoid 'idx' variable, forgot to say that the code was removed anyways - The whole algorithm for group_and_send_candidates was changed, any comments on the new algo ? - strict aliasing issues with g_has_table_iter_next? - Is it fine to not use gabble_generate_id? - How about the hard-coding special treatment in GabbleJingleContent? Should I find a better solution to all that ? - You got a solution to the re-entrance issues with gtalk_file_collection_terminate? - Merge jingle/share-Channel functionality into jingle-share ? - We forgot to discuss on how to handle SI vs. jingle-share decision in gabble. - other strict aliasing issue with content_new_channel_cb. That's it. Thanks again!
(In reply to comment #9) > > This could usefully be on a trivia branch that's merged ahead of the feature > > work. > If you want I could cherry pick it, but I don't think it's really necessary > atm, it's not like it's a bugfix or anything... The idea of having a trivia branch is that it gets the easy, insta-reviewable changes out of the way, so that the difficult-to-review branch is smaller. :-) > > > + /* This property is here only because jingle-session sets the media-type > > > + when constructing the object.. */ > > > + g_object_class_install_property (object_class, PROP_MEDIA_TYPE, > > > + g_param_spec_uint ("media-type", "media type", > > > > I thought the whole concept of media types was part of Jingle RTP, rather than > > the layer of Jingle at which file transfers appear? If I'm right, then file > > transfers should have media type JINGLE_MEDIA_TYPE_NONE? > > Well, the way I see it is that you have a 'media type file' for file transfers > (because, that's what it is, a ICE/UDP stream, in which you send a file). > But the reason this is there is explained in the comment. The jingle-session > creates the object with MEDIA_TYPE=MEDIA_TYPE_NONE, then it changes it > depending on the type of content in the jingle-session. There's just no > avoiding it with the way the jingle code works, at least, not without > refactoring it or adding special use cases, which I didn't want to do. Couldn't file transfers and other non-RTP contents have MEDIA_TYPE_NONE, at least? The reason inventing a spurious media type upsets me is that the way GabbleJingleSession creates objects with a media type at all is an RTP-specific wart: it's the Jingle-RTP (XEP-0167) media type, which ought to be local to GabbleJingleMediaRtp and the Telepathy media-channel stuff, leaking through into generic Jingle (XEP-0166) code like GabbleJingleSession. We tolerated it so far because *all* our Jingle contents were RTP, and it's harder to see domain-specific layering breakage when you only have one domain. In the long term, I think GabbleJingleSession shouldn't know about media types at all; in the short term, I think FTs (and Tubes) ought to either have no media type (NONE), or if that's impossible for some reason, they ought to share a new MEDIA_TYPE_NOT_MEDIA. If the Jingle session needs to treat RTP and FT contents differently (which it clearly does - e.g. adjusting @senders in gabble_jingle_content_parse_add), then that's probably a sign that the GabbleJingleContent base class doesn't have enough virtual methods (in this case the virtual method could be called "get_default_senders", perhaps?) > > > + if (error != NULL) > > > + g_set_error (error, TP_ERRORS, TP_ERROR_OFFLINE, > > > + "can't find contact's presence"); > > > > No need for the guard, g_set_error copes with NULL as a first argument (that's > > the whole point of using it instead of "*error = g_error_new ()") > > > > Don't know, that was copy/pasted code. Anyways, I won't "fix" this because I > prefer to have the guard than not have it. I insist. g_set_error's documentation *starts* with "Does nothing if err is NULL" and having the unnecessary guard will just make people wonder why this bit of code is somehow special (answer: actually it isn't). > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=2de68bb15afc8d1 > > > Remove checking for dialect in google transport > > > > What does this have to do with anything? The long commit message should explain > > why this change is desirable. > Oh, it's simple, it checks to see if the dialect is 'google' in the 'google > transport'.. it was not necessary and made no sense because the > google-transport is already only used when we are in google dialect. > Sorry, I thought it would be obvious. I'm glad I caught this, because it's (subtly!) wrong. GabbleJingleTransport is the gtalk-p2p transport (used as an alternative to ICE-UDP or raw UDP), whereas the dialect you removed a check for indicates which dialect of Jingle we're using. (The choice is currently between XEP-0166 draft 0.32, XEP-0166 draft 0.15, the Jingle variant from libjingle 0.4, or the Jingle variant from libjingle 0.3 - at some point we should add XEP-0166 v1.0, which is not 100% compatible with 0.32, to that list.) The official Google Talk clients will only ever use gtalk-p2p combined with some Google Jingle variant, but Gabble is quite capable of using standard XEP-0166 for the Jingle session, but the non-standard gtalk-p2p transport. Indeed, until version 0.7.something we didn't support either of the standard transports, and we still prefer to use gtalk-p2p if the target supports both gtalk-p2p and ICE-UDP (because that lets us use Google's relays, if we're signed in with a GMail account). > I wanted to make small patches, not necessary all fully working, that's why I > separated these two changes into two separate commits, to make it easier to > read/review/understand. I usually don't aim for each patch to be 'valid' but > rather to be 'isolated set of related changes'. The general rule of thumb is for commits to be as small as possible, but no smaller :-) > > > + GHashTable *channels; /* Google transport 'channels', not TP channels > > ... and why isn't it called components or component_ids, which would remove the > > confusion with Telepathy channels? (Youness and I agreed on IRC that we'd call these "share channels", since they're a concept specific to the Google file sharing (share-v1) protocol.) > > > + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &c)) > > > > This violates strict aliasing. Declare c with type gpointer; its real type will > > be obvious from context. > Humm, are you sure there will be strict aliasing errors with 'gpointer'? I'm > not getting any warning at compilation. Sadly, I think you're still violating strict aliasing rules. The compiler won't warn about it (because you shut it up with an explicit cast), but it might silently break your code, I think... The thing about strict aliasing (apart from the fact that hardly anyone fully understands it - I don't claim that I do!) is that it describes an assumption that the compiler is allowed to make in order to optimize the compiled code: given pointers A and B, if they point to "similar things" (like a struct _GArray and a const struct _GArray) the compiler pessimistically assumes that they may point to the same memory ("aliasing"), but if they point to "obviously different things" (like a struct _GObject and a struct _GArray) the compiler is allowed to assume that they don't point to the same memory. Unfortunately, while void * (and hence gpointer) is assumed to be able to alias any other type, the compiler may assume that a gpointer* (i.e. void**) and a GObject** do not alias. I just spotted that you're using the cast (gpointer) rather than (gpointer *), which means you *might* be safe here, but that's subtle, and complying with strict aliasing here is easy anyway. > Also, if I declare it as gpointer, then > I'd still need to cast it into GabbleJingleContent in order to call > gabble_jingle_content_parse_info Probably not, you can use a gpointer where any pointer is expected: const gchar *stoat_get_name (Stoat *self); gpointer p = g_object_new (STOAT_TYPE, NULL); Stoat *stoat; stoat_get_name (p); /* works */ stoat = p; /* works */ (Neither of these is valid without a cast in C++, however.) > > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=c66e8cb4b2fb > > > +static gchar * > > > +generate_temp_url (void) > > > > Couldn't you just use gabble_generate_id()? > didn't know it exists... but after looking at it, it seems to generate in a > format or "%lx.%lx/%lx/%lx" or uuid (I don't know the format of uuid), but > since I'm building a path here, I'm not sure it's a great idea to have '/' in > there.. i'd prefer just using the simple code I have. UUIDs look like this: 7c5d3d2d-f4cd-4284-926a-a2e4bbcb178d I don't really want different bits of Gabble creating miscellaneous assumed-unique IDs with different code: this is the sort of thing that ought to be in one place. It's a bit more subtle than it looks - for instance, you're concatenating integers with a variable width and no separator, which reduces the random space you're using by making e.g. (123, 45) equivalent to (12, 345) - and calling a library function seems to me to be the simplest thing possible here :-) I'd be happy to change gabble_generate_id to avoid slashes if that's necessary (underscores?), although slashes really aren't very magical in HTTP (as long as you don't have two consecutive slashes, /./ or /../, which these IDs don't). > But as an FYI, casting '42' as a gboolean is bad I agree, but if you can behave correctly for non-TRUE true values by typing 8 characters fewer (omitting " == TRUE"), it seems a win :-P (if() and while() would consider 42 to be a true value, so we should too.) > on some systems (can't remember if it > was AIX or MPE/x) TRUE is 0xFFFFFFFF, everything else is FALSE... > (which is also why we do '#define TRUE (1 == 1)', and not '#define TRUE 1') In GLib (and any sane C-based language), FALSE is 0, 0 is a false value, all nonzero integers are true values, and TRUE is an unspecified true value dependent on your compiler. The logic you describe isn't allowed by C99, since if (42) {...} is meant to execute the "...". (See §6.8.4.1 of WG14 N1124, available from <http://www.open-std.org/JTC1/SC22/wg14/www/standards>, if you care :-)
(In reply to comment #11) > > 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... This sounds wrong in any case - if you've pushed n bytes into the buffer, there's no guarantee that the client has read all those n bytes? I'm not sure how to solve this; perhaps file a separate bug and reference it in the comment. > > > +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.. This one definitely does violate the C99 aliasing rules (because it's gpointer* rather than gpointer), unless there's something I'm missing in the C99 standard - the cast just suppresses the warning, I think.
The quick checklist: Resolved ======== > - remove the check for error != NULL around g_set_error -> after discussion > with Will, I've fixed it. > - asked to avoid 'idx' variable, forgot to say that the code was removed > anyways All good then :-) Action from Youness =================== (In reply to comment #14) > - Do you still want me to cherry-pick the bytestrean/bytestream typo fix ? Yes please. If it passes make check, consider it to have been insta-reviewed. > - You still want me to remove/refactor the 'media-type' property ? I'd prefer FTs and (in future) Tubes to have MEDIA_TYPE_NONE, at least. > - strict aliasing issues with g_has_table_iter_next? > - other strict aliasing issue with content_new_channel_cb. See above > - Is it fine to not use gabble_generate_id? Not really (and it'll be a net code reduction), see above > - How about the hard-coding special treatment in GabbleJingleContent? Should I > find a better solution to all that ? Yes please; as I said above, I think a virtual method get_default_initiators() should be enough? Action from Simon ================= > - The whole algorithm for group_and_send_candidates was changed, any comments > on the new algo ? I'll have another look when I've gone through your fixes branch. > - You got a solution to the re-entrance issues with > gtalk_file_collection_terminate? Not really, I'll have another look when the branch stabilizes. Further discussion ================== > - Merge jingle/share-Channel functionality into jingle-share ? Not right now, probably, based on what you said on IRC? This can certainly wait til the branch has stabilized again. > - We forgot to discuss on how to handle SI vs. jingle-share decision in gabble. I think Jingle FTs should probably be preferred, but I'm not really sure; in the short term it depends how reliable they are :-) and in the medium term it depends how many people have (Google relays || working ICE) vs. SOCKS5 proxies (which probably means Jingle should be preferred, in practice). Perhaps leave it as-is for now, but file/reference a bug as I suggested? (In case it's not clear: I'm trying to impose the rule on all new code that every FIXME or TODO comment has an associated bug report. Otherwise, nobody will fix or do them (respectively), in practice :-)
(In reply to comment #14) > So, after a long discussion over IRC, here is a summary of the tasks that I > need to do to fix the various issues discussed previously : > - renaming GtalkFTManager into GTalkFileCollection DONE > - rename JingleChannel (and jingle channel references) into ShareChannel DONE > - move the manifest parsing into the GabbleFileTransferChannelManager and > remove the call to 'gabble_file_transfer_channel_new' from the > GTalkFileCollection DONE > - rename gabble_file_transfer_channel_set_gtalk_ft_state into > gabble_file_transfer_channel_gtalk_ft_state_changed to make it clear it's a > 'signal-like' API. DONE > - add comments explaining the design and layering violation, as well as the > protocol and object associations DONE (see http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=a946d89a524d8dcb2b80db353132ffdfe1cb795c ) sjoerd said it made sense.. > - remove GabbleChannel and use the channel's 'ACCEPTED' state for 'usable' and > use a GHashTable for the 'readable' fields of GabbleChannel. DONE > > > - Do you still want me to cherry-pick the bytestrean/bytestream typo fix ? DONE, make check worked (apart from the mail-notification test..) and considered insta-reviewed, so I've merged it into master > - You still want me to remove/refactor the 'media-type' property ? next on my TODO > - strict aliasing issues with g_has_table_iter_next? DONE, have a look at : http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=131531e300923cdabbbb852cdbe2e5618a371f15 > - Is it fine to not use gabble_generate_id? I'll use it and use something to remove non [a-zA-Z0-9] chars. > - How about the hard-coding special treatment in GabbleJingleContent? Should I > find a better solution to all that ? Will fix it. > - We forgot to discuss on how to handle SI vs. jingle-share decision in gabble. I think 'if you have turn set up, use jingle, otherwise SI', because even with libnice, in some cases it might fail. with SI, you always have some socks5 proxies, right? if not, then 'if no turn and no socks5 proxy, use jingle' I guess. > - other strict aliasing issue with content_new_channel_cb. DONE, have a look at it : http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=05d39c74f81e3a7e8944d02f93c6c622c5f8f2e9 I also reverted back the patch that removed the check for the google dialect in the jingle-transport-google. So for now we have : - Remove the MEDIA_TYPE for jingle-share - Remove special cases in jingle-content and add some new virtual methods for jingle-share. - use gabble_generate_id. - wrote decision algo on whether to use jingle or SI (right now, it uses jingle only if SI is not supported (gtalk)). - fix my TODO tags If you think I forgot something, let me know! Until I'm done and you go through your second pass review, you can already start reviewing my latest individual fixes from here : http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=shortlog;h=refs/heads/review-fixes p.s.: is it just me or has it become impossible to find something in this thread? :p
Changes look good up to <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=39066174ee3cc>, except that I'd like all the instances of gtalk_fc, GTALK_FC and (if any) GTalkFc to be gtalk_file_collection, etc. Still reviewing the subsequent patches... > So for now we have : > - Remove the MEDIA_TYPE for jingle-share > - Remove special cases in jingle-content and add some new virtual methods for > jingle-share. > - use gabble_generate_id. > - wrote decision algo on whether to use jingle or SI (right now, it uses jingle > only if SI is not supported (gtalk)). > - fix my TODO tags and also: * (me) finish reviewing your changes * (me) review HTTP implementation in detail * (me) second-pass review of the whole lot * (you) any further changes coming from those
Strict aliasing =============== <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=05d39c74f81e3a7> + union { + gpointer ptr; + GTalkFileCollection *self; + }; While very nice to use, this feature (anonymous unions) is unfortunately a gcc extension. Please be portable by giving the union a name (union {...} self;) and using data->self.ptr, data->self.self (perhaps rename the GTalkFileCollection member to fc, so it's data->self.fc). To be honest I'd be inclined to just make it: /* really a GTalkFileCollection, but, strict aliasing */ gpointer self; and use data->self where a gpointer would be acceptable (probably most places - remember that you can pass a gpointer to a function expecting a typed pointer, or assign a gpointer to a typed pointer variable), or GTalkFileCollection *self = data->self; blah_blah (self->priv->bees); in places where that won't work. Similarly, in the next patch: > + while (g_hash_table_iter_next (&iter, NULL, &value)) > { > + c = value; > gabble_jingle_content_parse_transport_info (c, node, error); you can just pass @value to g_j_c_p_t_i, and delete @c altogether? For the future ============== + /* the weakref to the channel here is held through the GList *channels */ A more concise wording (which also works for strong refs) would be "borrowed from @channels". + /* GList of weakreffed GabbleFileTransferChannel */ + /* GHashTable of GabbleFileTransferChannel => GINT_TO_POINTER (gboolean) */ You don't need to repeat the container type, which is obvious from the declaration - only the types/ownerships of the contents, which aren't.
(In reply to comment #19) > except that I'd like all the instances of gtalk_fc, GTALK_FC and (if any) > GTalkFc to be gtalk_file_collection, etc. To clarify that: I'd prefer properties and other public API to have the full name, and I'd prefer instance members (self->priv->gtalk_file_collection) to have the full name to be consistent with the properties they implement, but local variables can be gtalk_fc or fc or collection or something. The review-fixes branch looks good up to http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=131531e300923 (your last commit at the time of writing), apart from the things I mentioned. The documentation you added really helps - thanks!
(In reply to comment #20) > Strict aliasing > =============== > > <http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commitdiff;h=05d39c74f81e3a7> > + union { > + gpointer ptr; > + GTalkFileCollection *self; > + }; > > While very nice to use, this feature (anonymous unions) is unfortunately a gcc > extension. Please be portable by giving the union a name (union {...} self;) > and using data->self.ptr, data->self.self (perhaps rename the > GTalkFileCollection member to fc, so it's data->self.fc). Ah ok, I didn't know it wasn't standard, I'll fix it. > > To be honest I'd be inclined to just make it: > > /* really a GTalkFileCollection, but, strict aliasing */ > gpointer self; No, I don't really like doing that, i'll explain why below.. > > and use data->self where a gpointer would be acceptable (probably most places - > remember that you can pass a gpointer to a function expecting a typed pointer, > or assign a gpointer to a typed pointer variable), or > > GTalkFileCollection *self = data->self; > > blah_blah (self->priv->bees); > > in places where that won't work. > > Similarly, in the next patch: > > > + while (g_hash_table_iter_next (&iter, NULL, &value)) > > { > > + c = value; > > gabble_jingle_content_parse_transport_info (c, node, error); > > you can just pass @value to g_j_c_p_t_i, and delete @c altogether? No I didn't want to use value directly because I don't like using a gpointer like that. I prefer to have something with its type and use that rather than use an auto-cast-ed gpointer. Also, 'c' is used elsewhere in this function, so I can't remove it anyways, so I might as well use it. > > For the future > ============== > > + /* the weakref to the channel here is held through the GList *channels */ > > A more concise wording (which also works for strong refs) would be "borrowed > from @channels". > > + /* GList of weakreffed GabbleFileTransferChannel */ > + /* GHashTable of GabbleFileTransferChannel => GINT_TO_POINTER (gboolean) */ > > You don't need to repeat the container type, which is obvious from the > declaration - only the types/ownerships of the contents, which aren't. > I just find it easier to wrote "GHashTable of" rather than "a hash table of" and without the "a list of" or "a hash table of", I feel like the comment is 'empty' :p (In reply to comment #21) > (In reply to comment #19) > > except that I'd like all the instances of gtalk_fc, GTALK_FC and (if any) > > GTalkFc to be gtalk_file_collection, etc. > > To clarify that: I'd prefer properties and other public API to have the full > name, and I'd prefer instance members (self->priv->gtalk_file_collection) to > have the full name to be consistent with the properties they implement, but > local variables can be gtalk_fc or fc or collection or something. ok, I see.. I've fixed it now and used gtalk_file_collection everywhere. I prefered gtalk_fc because I hate long var names because of the 80char limit and I'm forced to split into two lines every method call using the variable... > > The review-fixes branch looks good up to > http://git.collabora.co.uk/?p=user/kakaroto/telepathy-gabble.git;a=commit;h=131531e300923 > (your last commit at the time of writing), apart from the things I mentioned. > The documentation you added really helps - thanks! > Thanks!
(In reply to comment #19) > > * (me) finish reviewing your changes > * (me) review HTTP implementation in detail > * (me) second-pass review of the whole lot > * (you) any further changes coming from those I've finished fixing everything, so all that remains is to review the branch again and (hopefully) merge it. However, I now have an additional branch 'igoogle' that has various fixes for iGoogle compatibility. It also contains some critical bugfixes to gtalk-ft which I can cherry-pick into the other branches if you think it's necessary.
Review of the rest of kakaroto/review-fixes: > + if (si_resource == NULL) > + si = FALSE; > else > - { > si = TRUE; > - } Trivial: si = (si_resource != NULL); would be more concise and (IMO) just as clear. > + /* Use bytestream if we have SI, but no jingle-share or if we have SI and > + jingle-share but we have no google relay token */ > + if (si && > + (!jingle_share || > + gabble_jingle_factory_get_google_relay_token ( > + self->priv->connection->jingle_factory) == NULL)) > + result = offer_bytestream (self, jid, si_resource, error); > + else if (jingle_share) > + result = offer_gtalk_file_transfer (self, jid, share_resource, error); > else > - result = offer_gtalk_file_transfer (self, jid, resource, error); > + { Trivial: {} around the two if clauses, please. I'll look at kakaroto/igoogle next.
> Add new VER hash for the unit test I was sure I complained about this before, but I can't find it. Anyway. Is it becoming clear that this is not how to do caps hashes? :-) caps_helper.py has code to calculate caps hashes: I'd much prefer it if these caps hashes were calculated from their contents, rather than being magic hard-coded values. Otherwise, the igoogle branch looks sane, so when I re-review I'll include that. I'll try to get the HTTP implementation reviewed soon, but the re-review of the whole branch might have to wait - I can't concentrate enough to actually spot problems if I do too much code-review at once :-)
(In reply to comment #25) > > Add new VER hash for the unit test > > I was sure I complained about this before, but I can't find it. Anyway. Is it > becoming clear that this is not how to do caps hashes? :-) > > caps_helper.py has code to calculate caps hashes: I'd much prefer it if these > caps hashes were calculated from their contents, rather than being magic > hard-coded values. > > Otherwise, the igoogle branch looks sane, so when I re-review I'll include > that. > > I'll try to get the HTTP implementation reviewed soon, but the re-review of the > whole branch might have to wait - I can't concentrate enough to actually spot > problems if I do too much code-review at once :-) hehe, no you never complained about this, although I was expecting you to :p And yes, I know there's a method in caps_helper to create the hash, however, I can't think of a proper way to do this.. the reason being that I get one hash, and I need to create a new hash with the same features minus the SI file transfer ones.. since I don't know what features are already available, then there's no way for me to calculate the new hash after I remove from it a feature.. and since I need to hijack that hash at the moment of sending the presence, it has to be done before the caps discovery request is sent... If you have any idea, let me know! (I'm thinking just now about doing something like : get the hash+features from before we add FT caps and store them, then add the FT caps, and calculate the hash from the old feature + share-v1.. do you see this as a working solution or you see a possible issue with that?)
(In reply to comment #26) > (I'm thinking just now about doing something like : get the hash+features from > before we add FT caps and store them, then add the FT caps, and calculate the > hash from the old feature + share-v1.. do you see this as a working solution or > you see a possible issue with that?) That'd probably work. Or, use the fact that caps_helper.py already "knows" (i.e. hard-codes) what "fixed" caps Gabble ought to have, and base your logic on that? (For the outgoing caps and caps-hash tests to work and be useful, the test suite already needs to hard-code what "fixed" caps Gabble ought to have, before optional caps are added by Telepathy.)
Here's a review of the HTTP part; after you've responded to this and the hashes in the tests, hopefully someone else (Sjoerd?) will do the re-review pass, to be more likely to catch things I've missed. > static void get_next_manifest_entry (GTalkFileCollection *self, > ShareChannel *share_channel, gboolean error) (Whitespace: static void \n get_next... please.) > filename = g_strdup_printf ("%s%s", > entry->name, entry->folder? ".tar":""); I'd prefer more space, and perhaps parentheses, around the ternary operator to make it clearer what's going on. > guint url_len = source_url != NULL? strlen (source_url) : 0; I'd prefer "... = (source_url != NULL ? strlen (source_url) : 0);", and similar in the g_strdup_printf() call for the GET. > "Host: %s:0\r\n" > "User-Agent: %s\r\n\r\n", The Host is rather non-obvious: I take it the strange Host header is part of the Google Talk HTTP-over-pseudoTCP protocol? I'd prefer this commented, particularly since it's non-obvious whether it includes a resource (in fact it does): "Host: %s:0\r\n" /* e.g. alice@example.com/Empathy:0 */ > /* Return the pointer at the end of the line or NULL if not \n found */ > static gchar * > http_read_line (gchar *buffer, guint len) This looks like it should be called cut_off_first_line or something? A better comment, IMO, would be: /* If buffer contains a line ending, 0-terminate the first line and * return a pointer to the beginning of the next line. Otherwise * return NULL. */ Anything dealing with memory block sizes (like this function) should ideally take/return a gsize, which can be bigger than guint on 64-bit, rather than a guint (although in practice if you're using IM file transfer for more than 2GB it's not going to work :-) I'd prefer '\0' rather than 0 when 0-terminating strings. > DEBUG ("Found server headers line (%d) : %s", strlen (line), line); > DEBUG ("Found client headers line (%d) : %s", strlen (line), line); strlen returns a size_t. The printf format for printing a size_t is "%" G_GSIZE_FORMAT - this matters on LP64 platforms, like amd64, where sizeof (size_t) == sizeof (void *) == 8 and sizeof (int) == 4. In http_data_received (twice, for client and server): > if (*line == 0) I'd prefer '\0', and I think line[0] would be a clearer notation too. > "Content-Length: %llu\r\n" "Content-Length: %" G_GUINT64_FORMAT "\r\n", please. > DEBUG ("Unable to find valid filename (%s), result : 404", > filename); I think this should printf status_line, not filename, because otherwise if the scanf fails we printf NULL; on (at least) Solaris that crashes in libc, and on platforms with a non-crackful libc the debug message "Unable to find valid filename ((null))" isn't very helpful. > else if (!g_ascii_strncasecmp (line, > "Transfer-Encoding: chunked", 26)) I think you mean 27 - you want the '\0' at the end to match too, to avoid matching "Transfer-Encoding: chunked-but-not-as-we-know-it" (but actually, what you really want is plain g_ascii_strcasecmp, since both strings are '\0'-terminated, and this would avoid hard-coding the length). > if (g_str_has_prefix (share_channel->status_line, > "HTTP/1.1 200")) If the status isn't 200, it can still have a body, so you need to skip Content-Length bytes of 404 response, or a stream of chunks. Use the same code but set a boolean "skipping" flag, perhaps? > /* we assume http_data_received never returns consumed > len */ Looks like a job for g_assert()?
(In reply to comment #28) > Here's a review of the HTTP part; after you've responded to this and the hashes > in the tests, hopefully someone else (Sjoerd?) will do the re-review pass, to > be more likely to catch things I've missed. Thanks.. I'll do the hashes thing later... > > > static void get_next_manifest_entry (GTalkFileCollection *self, > > ShareChannel *share_channel, gboolean error) > > (Whitespace: static void \n get_next... please.) oups, fixed > > > filename = g_strdup_printf ("%s%s", > > entry->name, entry->folder? ".tar":""); > > I'd prefer more space, and perhaps parentheses, around the ternary operator to > make it clearer what's going on. done > > > guint url_len = source_url != NULL? strlen (source_url) : 0; > > I'd prefer "... = (source_url != NULL ? strlen (source_url) : 0);", and similar > in the g_strdup_printf() call for the GET. done > > > "Host: %s:0\r\n" > > "User-Agent: %s\r\n\r\n", > > The Host is rather non-obvious: I take it the strange Host header is part of > the Google Talk HTTP-over-pseudoTCP protocol? I'd prefer this commented, > particularly since it's non-obvious whether it includes a resource (in fact it > does): > > "Host: %s:0\r\n" /* e.g. alice@example.com/Empathy:0 */ I added that example comment, but this isn't important.. that Host is what libjingle 4.0 uses, but anything can be put there and it doesn't affect it... (iGoogle for example doesn't send a Host header, just a single line GET with no headers). > > > /* Return the pointer at the end of the line or NULL if not \n found */ > > static gchar * > > http_read_line (gchar *buffer, guint len) > > This looks like it should be called cut_off_first_line or something? A better > comment, IMO, would be: I'm not sure about changing the name of the function, I find it ok as it is. If you insist, I'll change it. > > /* If buffer contains a line ending, 0-terminate the first line and > * return a pointer to the beginning of the next line. Otherwise > * return NULL. */ I changed the comment to match yours since it's better, thanks > > Anything dealing with memory block sizes (like this function) should ideally > take/return a gsize, which can be bigger than guint on 64-bit, rather than a > guint (although in practice if you're using IM file transfer for more than 2GB > it's not going to work :-) hehe, no, it's a guint because it's used by the received_http_data which has a 'guint len' argument, which itself is from what the nice_data_received which is also called with a guint... The data received is supposed to be for one udp packet, so it would never be higher than the max size of a UDP packet (65535), so it's not gsize, it's a guint and it's more than enough (it could even work with gshort, but since we could have extra data from a previous unfinished line, it could exceed gshort, so guint is safe). > > I'd prefer '\0' rather than 0 when 0-terminating strings. done > > > DEBUG ("Found server headers line (%d) : %s", strlen (line), line); > > DEBUG ("Found client headers line (%d) : %s", strlen (line), line); > > strlen returns a size_t. The printf format for printing a size_t is "%" > G_GSIZE_FORMAT - this matters on LP64 platforms, like amd64, where sizeof > (size_t) == sizeof (void *) == 8 and sizeof (int) == 4. > > In http_data_received (twice, for client and server): indeed, I fixed those, thanks > > > if (*line == 0) > > I'd prefer '\0', and I think line[0] would be a clearer notation too. done > > > "Content-Length: %llu\r\n" > > "Content-Length: %" G_GUINT64_FORMAT "\r\n", please. done > > > DEBUG ("Unable to find valid filename (%s), result : 404", > > filename); > > I think this should printf status_line, not filename, because otherwise if the > scanf fails we printf NULL; on (at least) Solaris that crashes in libc, and on > platforms with a non-crackful libc the debug message "Unable to find valid > filename ((null))" isn't very helpful. Ok, I fixed this by checking for filename?filename:"" instead, the purpose of this message was to print what filename was scanned, but the status line is already printed in the DEBUG line just above it, so we get both status_line *and* scanned filename. But a NULL-check was indeed needed. > > > else if (!g_ascii_strncasecmp (line, > > "Transfer-Encoding: chunked", 26)) > > I think you mean 27 - you want the '\0' at the end to match too, to avoid > matching "Transfer-Encoding: chunked-but-not-as-we-know-it" (but actually, what > you really want is plain g_ascii_strcasecmp, since both strings are > '\0'-terminated, and this would avoid hard-coding the length). Yeah, 27 I guess, I made it into strcasecmp instead as suggested (for Content-Length too). thanks > > > if (g_str_has_prefix (share_channel->status_line, > > "HTTP/1.1 200")) > > If the status isn't 200, it can still have a body, so you need to skip > Content-Length bytes of 404 response, or a stream of chunks. Use the same code > but set a boolean "skipping" flag, perhaps? Actually no, there won't be any body.. although yes it could, it will never in our case.. don't forget that this isn't an HTTP library, it's a subset of HTTP for the purpose of libjingle compat.. and libjingle will never send a body with a 404 error. Anyways, I added a g_assert there to make sure it never happens.. I don't really like the idea of adding extra code that isn't necessary. (Same goes for the request.. it could be a POST with a body, but we never check the body when receiving a request.. it's a FIXME in there, but it never needs to be done since we're doing a subset of HTTP, not a real HTTP library). > > > /* we assume http_data_received never returns consumed > len */ > > Looks like a job for g_assert()? Added g_assert
(In reply to comment #29) > Actually no, there won't be any body.. although yes it could, it will never in > our case.. don't forget that this isn't an HTTP library, it's a subset of HTTP > for the purpose of libjingle compat.. and libjingle will never send a body with > a 404 error. Anyways, I added a g_assert there to make sure it never happens.. An assertion isn't appropriate here, because it's remotely-triggerable. Please use g_warning() and terminate the channels, or something.
(In reply to comment #30) > (In reply to comment #29) > > Actually no, there won't be any body.. although yes it could, it will never in > > our case.. don't forget that this isn't an HTTP library, it's a subset of HTTP > > for the purpose of libjingle compat.. and libjingle will never send a body with > > a 404 error. Anyways, I added a g_assert there to make sure it never happens.. > > An assertion isn't appropriate here, because it's remotely-triggerable. Please > use g_warning() and terminate the channels, or something. oups.. /me feels shame.. worst part is, I realized this, but then completely forgot about it. Anyways, it now does a debug message and errors out all channels.
This was merged in 0.9.13.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.