Bug 32612

Summary: Remove Tubes code
Product: Telepathy Reporter: Jonny Lamb <jonny.lamb>
Component: gabbleAssignee: Jonny Lamb <jonny.lamb>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~jonny/telepathy-gabble/log/?h=only-one-tube
Whiteboard: review+
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 37630, 47760, 50828, 54061    

Description Jonny Lamb 2010-12-23 09:49:41 UTC
Tubes are old and crap and pretty much nothing uses them anymore as uwog has ported abiword and tomeu ported most of sugar, or something. They make lots of gabble code very complicated, so let's remove them! All in favour say aye.
Comment 1 Jonny Lamb 2010-12-24 03:19:55 UTC
(In reply to comment #0)
> and tomeu ported most of sugar, or something.

Not entirely true. I think he ported away from presence service or something, but activities are still very much using the old Tubes API.

Personally, I'm tempted to tell them just to jfdi when it comes to porting to the new API as it's been around for like two years or something and we have deprecated Tubes. We're also coming up to an spec/API break soon...

Y/N?
Comment 2 Simon McVittie 2011-01-03 04:01:54 UTC
Is there any point in deleting it before we do the Glorious 1.0 API Break and break everyone anyway?

The intention is that the 1.0 API doesn't necessarily interoperate with 0.x (I'd be very tempted to force it to *never* interoperate by calling it o.fd.Telepathy1, to avoid confusion) and I suspect most of Sugar still hard-codes misc interface names, so porting will be needed in any case.
Comment 3 Jonny Lamb 2011-01-03 06:22:42 UTC
(In reply to comment #2)
> Is there any point in deleting it before we do the Glorious 1.0 API Break and
> break everyone anyway?

My intention was to make the transitioning from GabbleMucChannel to GabbleTextMucChannel (and friends) easier. I started doing this a while ago but got thwarted by Telepathy properties being necessary in channels which weren't text. Hopefully that'll be fixed soon though.

Well, yes, it appears we can't really do this now, which is a shame. :-(
Comment 4 Jonny Lamb 2012-06-07 06:08:55 UTC
Here's a patch. I threw the 'only expose the text channel on the bus when necessary' stuff on top.
Comment 5 Simon McVittie 2012-07-18 10:59:26 UTC
I'm reviewing patch-by-patch, so it's possible that some of my review comments are fixed in a later patch.

"private-tubes-factory: create only Tube channels for requests"

+gabble_private_tubes_factory_lookup (GabblePrivateTubesFactory *self,
...
+ g_object_get (tube,
+ "channel-type", &channel_type,
+ "handle", &channel_handle,
+ "service", &channel_service,
+ NULL);
+
+ if (!tp_strdiff (type, channel_type)
+ && handle == channel_handle
+ && !tp_strdiff (service, channel_service))
+ match = FALSE;

Isn't this backwards? It should set match = TRUE if everything matches.

(This essentially gives all requests for a private tube "create" semantics, but it looks as though pre-existing bugs in the _requestotron implementation gave them those semantics anyway...)

> +new_channel_from_request (GabblePrivateTubesFactory *self,

I think this deserves Returns: (transfer none) in a pseudo-doc-comment.

+ g_hash_table_insert (channels, channel, request_tokens);
+ tp_channel_manager_emit_new_channels (self, channels);
+
+ g_hash_table_unref (channels);

The GHashTable is no longer needed. Just use tp_channel_manager_emit_new_channel (self, channel, request_tokens).
Comment 6 Simon McVittie 2012-07-18 11:18:04 UTC
"private-tubes-factory: only create Tube channels for tube offers"

+ guint tube_id;
...
+ tube_id = g_ascii_strtoull (tmp, NULL, 10);
...
+ if (tube_id == 0 || tube_id >= G_MAXINT)
+ DEBUG ("tube ID is not numeric or > 2**32: %s", tmp);

tube_id should be a guint64 really, and the comparison should be with G_MAXUINT32 (unless you really mean G_MAXINT32, and 2**31 in the debug message). This is probably not your fault, you probably moved the buggy code from elsewhere.

The debug message is misleading, since 0 is also disallowed. "tube ID is non-numeric or out of range"?

+ DEBUG ("tube ID already in use; do not open the offered tube and close "
+ "the existing tube if it's to the same contact");

Not a merge blocker and presumably not your fault, but these semantics are crazy. We should have a separate tube ID "namespace" per peer, and store tubes in the hash table by (handle, id) tuples or something.

+private_tubes_factory_tube_close_cb (
...
+ if (!tube_msg_checks (self, msg, node, NULL, &tube_id))
+ return FALSE;

Er, this function allows Alice to close tubes between us and Bob, if she can guess or brute-force the tube ID. Pre-existing bug?

+new_channel_from_stanza (GabblePrivateTubesFactory *self,

Deserves a Returns: (transfer none) pseudo-doc-comment, I think - its name sounds as though it might return a ref.

+ /* this has already been checked */
+ handle = tp_handle_lookup (contact_repo,
+ wocky_stanza_get_from (stanza), NULL, NULL);

Has it really? Validity has been checked, but existence of a handle hasn't. extract_tube_information doesn't seem to ensure the handle if the initiator_handle "out" parameter is NULL, as it is here.

I think you should use tp_handle_ensure, then assert that the handle is nonzero (which, you're right, Wocky should already have checked).
Comment 7 Simon McVittie 2012-07-18 11:21:13 UTC
"private-tubes-factory: implement handle_si_stream_request"

+ tube_id_tmp = strtoul (tmp, &endptr, 10);

g_ascii_strtoull, please, and make tube_id_tmp a guint64.
Comment 8 Simon McVittie 2012-07-18 11:23:40 UTC
"private-tubes-factory: move SI tube request handling to here"

+ si_node = wocky_node_get_child_ns (
+ wocky_stanza_get_top_node (stanza), "si", NS_SI);
+ stream_id = wocky_node_get_attribute (si_node, "id");

Could do with some assertions.
Comment 9 Simon McVittie 2012-07-18 11:24:52 UTC
"private-tubes-factory: make error messages less specific"

... "more specific"
Comment 10 Simon McVittie 2012-07-18 11:39:29 UTC
(In reply to comment #5)
> I'm reviewing patch-by-patch, so it's possible that some of my review comments
> are fixed in a later patch.

I think I've now reviewed enough (up to "private-tubes-factory: make extract_tube_information public") that you could fix some of the review comments and merge the 1-1 tube bits, if you want.

Overall, the 1-1 tube stuff looks good, and I agree we should finally make this happen. My review comments are pretty minor and should be easy to fix.

Having a single "namespace" for tube IDs even between multiple peers is a design flaw, presumably a pre-existing one, but we can fix that separately (and make sure that if/when we do stream tubes via Jingle, we don't inherit the design flaw). I suggest opening a separate bug for this.

> Er, this function allows Alice to close tubes between us and Bob, if she can
> guess or brute-force the tube ID. Pre-existing bug?

This is also not a merge blocker if it's a pre-existing bug, but if you don't fix it, please open a separate bug for it.
Comment 11 Simon McVittie 2012-07-18 12:36:58 UTC
"muc-channel: add way to request single Tube channels"

+ do
+ {
+ out = g_random_int_range (0, G_MAXINT);
+ }
+ while (g_hash_table_lookup (priv->tubes,
+ GUINT_TO_POINTER (out)) != NULL);

should be in range (1 to G_MAXINT32) since 0 seems to be considered invalid (this might apply to private tubes too, actually).
Comment 12 Simon McVittie 2012-07-18 12:41:40 UTC
"tubes test: fix up now that we return text & tube channels together"

Isn't this about to be reverted? It seems as though it'd be better to keep the old signalling.
Comment 13 Simon McVittie 2012-07-18 12:43:17 UTC
"muc-channel: enable SI stream requests again"

> + tube_id_tmp = strtoul (tmp, &endptr, 10);

Same problems as for 1-1 tubes.
Comment 14 Simon McVittie 2012-07-18 13:01:52 UTC
"muc-channel: disappear from the bus when not requested"

+ DEBUG ("making MUC channel reappear!");
+ tp_base_channel_reopened_with_requested (base, FALSE, sender);
+ g_signal_emit (chan, signals[APPEARED], 0);

I can't help thinking that if the appeared signal is necessary in Gabble, then telepathy-glib is not being sufficiently helpful for Bug #48210.

Couldn't muc_channel_closed_cb handle the reappearance if (channel.is_respawning()) like I suggested in Bug #48210 Comment #6? (Yes, I realise this is pretty strange.)

Alternatively, TpExportableChannel or TpBaseChannel could grow a reappeared signal or something?

+ gboolean needed_not_wanted,

If you reverse the sense of this flag, and call it export_text or something, then things might be a bit more natural:

+ if (gabble_muc_channel_get_autoclose (*ret))
+ gabble_muc_channel_set_autoclose (*ret, needed_not_wanted);

could become

    if (export_text)
      gabble_muc_channel_set_autoclose (FALSE);

which I think is maybe clearer.
Comment 15 Simon McVittie 2012-07-18 15:36:51 UTC
I think I've covered everything. This is really close to ready-to-merge, so I'll hold off on reviewing the Salut version on the basis that it's probably roughly the same code.
Comment 16 Jonny Lamb 2012-07-24 16:06:18 UTC
Okay I think I fixed everything you said, and opened bug #52448 about this:

(In reply to comment #6)
> Not a merge blocker and presumably not your fault, but these semantics are
> crazy. We should have a separate tube ID "namespace" per peer, and store tubes
> in the hash table by (handle, id) tuples or something.
[...]
> Er, this function allows Alice to close tubes between us and Bob, if she can
> guess or brute-force the tube ID. Pre-existing bug?

I pushed fixes as new commits at the top of the branch for ease of review, apart from the one where I needed to change the commit message.

╻  ╻╻┏ ┏━╸   ╺┳╸┏━┓╺┳╸┏━┓╻  ╻  ╻ ╻
┃  ┃┣┻┓┣╸     ┃ ┃ ┃ ┃ ┣━┫┃  ┃  ┗┳┛
┗━╸╹╹ ╹┗━╸    ╹ ┗━┛ ╹ ╹ ╹┗━╸┗━╸ ╹ 

┏━┓╻ ╻┏━╸┏━┓┏━┓┏┳┓┏━╸   ╻  ┏━┓╻  
┣━┫┃╻┃┣╸ ┗━┓┃ ┃┃┃┃┣╸    ┃  ┃ ┃┃  
╹ ╹┗┻┛┗━╸┗━┛┗━┛╹ ╹┗━╸   ┗━╸┗━┛┗━╸
Comment 17 Simon McVittie 2012-08-14 15:16:57 UTC
Looks OK now, ship it (when you've added a dependency on the telepathy-glib that closes Bug #48210).
Comment 18 Jonny Lamb 2012-08-28 13:33:01 UTC
┏━┓┏┳┓┏━╸   ╺┳┓┏━┓┏┓╻┏━╸
┃ ┃┃┃┃┃╺┓    ┃┃┃ ┃┃┗┫┣╸ 
┗━┛╹ ╹┗━┛   ╺┻┛┗━┛╹ ╹┗━╸

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.