Bug 33911

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…

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.

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

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

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

Looks fine. I guess there's still more work to be done though...

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?

I may have also accidentally jingle-*; and also lm_message_[un]ref.

Aiiiight I merged that bit. There's still a lot of Loudmouth to go, but:

 72 files changed, 1459 insertions(+), 2495 deletions(-)

I'm just leaving this here…

 72 files changed, 1090 insertions(+), 2013 deletions(-)

 * [new branch]      quietmouth -> wjt/quietmouth

...

http://i.imgur.com/DKgXF.png

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.

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

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.

(I'll leave 'node-iter-get-child-with-namespace' for another day.)

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

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

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.

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

I'm so happy that this bug is close to being closed!

I wonder if 0.15.4 will get loads of regression reports…