Bug 32612

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

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.

(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. :-(

Here's a patch. I threw the 'only expose the text channel on the bus when necessary' stuff on top.

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).

"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).

"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.

"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.

"private-tubes-factory: make error messages less specific"

... "more specific"

(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.

"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).

"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.

"muc-channel: enable SI stream requests again"

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

Same problems as for 1-1 tubes.

"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.

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.

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.

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

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

Looks OK now, ship it (when you've added a dependency on the telepathy-glib that closes Bug #48210).

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