Bug 33911

Summary: Remove Loudmouth wrapper
Product: Telepathy Reporter: Jonny Lamb <jonny.lamb>
Component: gabbleAssignee: Telepathy bugs list <telepathy-bugs>
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.collabora.com/git/user/wjt/telepathy-gabble/log/?h=node-iter-get-child-with-namespace
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 34716, 45400    
Bug Blocks: 34975    
Attachments: disco: don't crash on replies whose <query/> is missing

Description Jonny Lamb 2011-02-04 09:12:58 UTC
I've started working on this, it's taken longer than I thought it'd take.

I tried splitting up my patches as much as I could but then it seemed like it was taking much longer to stage patch-by-patch than it would for a reviewer to believe I can use sed and can type "make check".

I'll add the patch keyword when it's all done. So far I've just removed LmMessageNode.
Comment 1 Will Thompson 2011-02-28 13:27:54 UTC
I've rebased this onto my branch from bug 34716, and reverted the over-zealous sedding that led to the incredible wocky_node_add_child_with_contentren_from_properties() being defined in util.c.

It looks fine. More patches to come…
Comment 2 Will Thompson 2011-05-02 08:25:11 UTC
yum yum rebased.


 68 files changed, 1382 insertions(+), 2400 deletions(-)


I think if we aim to do this all in one go it'll never happen.
Comment 3 Jonny Lamb 2011-05-02 08:53:20 UTC
- result = wocky_stanza_build_iq_result (stanza, NULL);
-
- wocky_porter_send (porter, result);
-
- g_object_unref (result);
+ wocky_porter_acknowledge_iq (porter, stanza, NULL);
return TRUE;
}
@@ -879,17 +874,19 @@ iq_privacy_list_push_cb (LmMessageHandler *handler,
!list_node)
return LM_HANDLER_RESULT_ALLOW_MORE_HANDLERS;
- result = lm_iq_message_make_result (message);
+ result = wocky_stanza_build_iq_result (message, NULL);
- wocky_porter_send (wocky_session_get_porter (conn->session), result);
+ if (result != NULL)
+ {
+ wocky_porter_send (wocky_session_get_porter (conn->session), result);
+ g_object_unref (result);
+ }

Why do you use acknowledge_iq in the first case, but build_iq_result and then immediately send it in the second case?

(I've not reviewed your top patch removing GabbleXmppError yet because I saw it and I tl;dr'd it for now.)
Comment 4 Jonny Lamb 2011-05-03 01:12:32 UTC
(Why can't I copy paste spaces today I wonder?

+  if (vcard_error->domain == WOCKY_XMPP_ERROR)
+    if (vcard_error->code == WOCKY_XMPP_ERROR_BAD_REQUEST ||
+        vcard_error->code == WOCKY_XMPP_ERROR_NOT_ACCEPTABLE)
       tp_error.code = TP_ERROR_INVALID_ARGUMENT;

Can't say I approve of this, but yeah it's not new code.

  DEBUG ("fetching location failed: %s", wocky_error->message);
+ /* FUUCK */
  gabble_set_tp_error_from_wocky (wocky_error, &tp_error);

Hm?

- GError *error = gabble_message_get_xmpp_error (reply);
+ GError *error = NULL;
+
+ /* FIXME: did anything depend on getting errors outside core? */
+ wocky_stanza_extract_errors (reply, NULL, &error, NULL, NULL);

This is not a regression though, is it?

- /* FIXME: add a helper in error.c to extract the type, error, and message
- * from an XMPP stanza.
- */

Woo!

Basically fine though.
Comment 5 Will Thompson 2011-05-03 05:18:33 UTC
(In reply to comment #4)
> (Why can't I copy paste spaces today I wonder?
> 
> +  if (vcard_error->domain == WOCKY_XMPP_ERROR)
> +    if (vcard_error->code == WOCKY_XMPP_ERROR_BAD_REQUEST ||
> +        vcard_error->code == WOCKY_XMPP_ERROR_NOT_ACCEPTABLE)
>        tp_error.code = TP_ERROR_INVALID_ARGUMENT;
> 
> Can't say I approve of this, but yeah it's not new code.

I guess it should probably use gabble_set_tp_error_from_wocky() but I didn't really want to go down the road of changing everything to use this just yet.

>   DEBUG ("fetching location failed: %s", wocky_error->message);
> + /* FUUCK */
>   gabble_set_tp_error_from_wocky (wocky_error, &tp_error);
> 
> Hm?

Ahem. I don't know where that came from. Amended.

> 
> - GError *error = gabble_message_get_xmpp_error (reply);
> + GError *error = NULL;
> +
> + /* FIXME: did anything depend on getting errors outside core? */
> + wocky_stanza_extract_errors (reply, NULL, &error, NULL, NULL);
> 
> This is not a regression though, is it?

It might be: gabble_message_get_xmpp_error() parsed Jingle and SI errors as well as just XMPP core errors. So I have just checked everything that uses the request pipeline (whether directly or indirectly) and nothing was relying on this.

Pushed a patch removing the FIXME!
Comment 6 Jonny Lamb 2011-05-03 05:25:27 UTC
Looks fine. I guess there's still more work to be done though...
Comment 7 Will Thompson 2012-01-26 05:14:52 UTC
Ahem.

I pushed a commit to http://cgit.collabora.com/git/user/wjt/telepathy-gabble-wjt.git/commit/?h=BYE-BYE-LOUDMOUTH merging master into this branch. It builds, tests pass. You okay with me pushing it?
Comment 8 Will Thompson 2012-01-26 06:37:38 UTC
I may have also accidentally jingle-*; and also lm_message_[un]ref.
Comment 9 Will Thompson 2012-01-26 08:19:00 UTC
Aiiiight I merged that bit. There's still a lot of Loudmouth to go, but:

 72 files changed, 1459 insertions(+), 2495 deletions(-)
Comment 10 Will Thompson 2012-01-27 14:17:02 UTC
I'm just leaving this here…

 72 files changed, 1090 insertions(+), 2013 deletions(-)
Comment 11 Jonny Lamb 2012-01-27 14:45:07 UTC
 * [new branch]      quietmouth -> wjt/quietmouth

...

http://i.imgur.com/DKgXF.png
Comment 12 Will Thompson 2012-01-28 04:58:12 UTC
Created attachment 56254 [details] [review]
disco: don't crash on replies whose <query/> is missing

This is a regression from the BYE-BYE-LOUDMOUTH branch. The logic in
request_reply_cb is meant to be:

• if the stanza is an error, pass (with 'err' having been filled in);
• else, if there is no <query/> node in the reply, set 'err' to that
  effect;
• now call the callback; it is guaranteed that if err is NULL,
  query_node is not NULL.

I inverted the check for the return value of
wocky_stanza_extract_errors(): it returns TRUE if 'reply_msg' was an
error, not FALSE.
Comment 13 Will Thompson 2012-01-30 09:33:47 UTC
Here's a follow-up branch which removes the NodeIter compatibility layer in favour of WockyNodeIter, and removes most uses of the dreaded lm_message_node_get_child_with_namespace. It uses a few new Wocky patches from bug 45400.

http://cgit.collabora.com/git/user/wjt/telepathy-gabble-wjt.git/log/?h=node-iter-get-child-with-namespace

 35 files changed, 402 insertions(+), 478 deletions(-)
Comment 14 Jonny Lamb 2012-01-30 12:54:00 UTC
Okay, well, I looked over this huge branch. I'm not going to claim it's the most thorough review ever but I trust your ability to run `make check`, and really, what's the worst that can happen. Only one question:

- else if (lm_message_get_sub_type (msg) != LM_MESSAGE_SUB_TYPE_RESULT)
+ else if (wocky_stanza_extract_errors (msg, NULL, NULL, NULL, NULL))

These two aren't really equal though, are they?

Serious hats off for doing this though; it must've been well boring.
Comment 15 Jonny Lamb 2012-01-30 12:54:29 UTC
(I'll leave 'node-iter-get-child-with-namespace' for another day.)
Comment 16 Will Thompson 2012-01-31 07:22:03 UTC
(In reply to comment #14)
> Okay, well, I looked over this huge branch. I'm not going to claim it's the
> most thorough review ever but I trust your ability to run `make check`, and
> really, what's the worst that can happen. Only one question:
> 
> - else if (lm_message_get_sub_type (msg) != LM_MESSAGE_SUB_TYPE_RESULT)
> + else if (wocky_stanza_extract_errors (msg, NULL, NULL, NULL, NULL))
> 
> These two aren't really equal though, are they?

In general, no, but here they are. If you know that 'msg' is a reply to an IQ, then you know that it either has type='result' or type='error'. So:

    wocky_stanza_extract_errors (msg, ...) returns TRUE
  ⇔
    lm_message_get_sub_type (msg) == LM_MESSAGE_SUB_TYPE_ERROR
  ⇔
    lm_message_get_sub_type (msg) != LM_MESSAGE_SUB_TYPE_RESULT

It's true that wocky_stanza_extract_errors() does a bit more rummaging around in the stanza, even if all its out parameters are %NULL. I could make it short-circuit… or add a wocky_stanza_is_error () maybe?
Comment 17 Will Thompson 2012-01-31 07:23:00 UTC
(In reply to comment #14)
> Serious hats off for doing this though; it must've been well boring.

Whenever I deleted a lm_* function, I drank a shot.
Comment 18 Will Thompson 2012-01-31 09:30:00 UTC
So… I merged quietmouth. Commence screaming.

Updating the URL to point at node-iter-get-child-with-namespace (and also to reflect that I renamed all my Git repositories) for Jonny's later perusal.
Comment 19 Simon McVittie 2012-01-31 10:21:54 UTC
(In reply to comment #18)
> So… I merged quietmouth. Commence screaming.
> 
> Updating the URL to point at node-iter-get-child-with-namespace (and also to
> reflect that I renamed all my Git repositories) for Jonny's later perusal.

I like the look of this branch.

> conn-olpc: give _get_child_with_namespace a clearer name

If I was in your place I'd have been very tempted to rename it to kill_kittens()...
Comment 20 Jonny Lamb 2012-01-31 11:13:42 UTC
I'm so happy that this bug is close to being closed!
Comment 21 Will Thompson 2012-02-01 03:24:11 UTC
I wonder if 0.15.4 will get loads of regression reports…

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.