Bug 26272 - Improve acknowledging and nacking IQs
Summary: Improve acknowledging and nacking IQs
Status: RESOLVED FIXED
Alias: None
Product: Wocky
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/wj...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-01-27 07:03 UTC by Will Thompson
Modified: 2011-03-10 02:52 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Will Thompson 2010-01-27 07:03:17 UTC
XMPP Core requires that <iq/>s have an id='' attribute (in the first bullet point of <http://xmpp.org/rfcs/rfc3920.html#stanzas-semantics-iq>). Thus, Wocky would be justified in silently dropping incoming IQs that don't follow this rule.

(Per bug 26271, at least ejabberd happily lets such IQs through...)
Comment 1 Will Thompson 2011-03-02 03:23:06 UTC
No, we shouldn't: Rob's pointed out that some servers send roster pushes with no id='', and we shouldn't drop those.

We should, however, make it easier for code to cope with such stanzas, by making build_iq_reply and friends not assert in this case. Branch coming up to improve acking/naking in general…
Comment 2 Will Thompson 2011-03-02 07:08:14 UTC
Here's a WIP branch. It is on top of <http://git.collabora.co.uk/?p=user/wjt/wocky.git;a=shortlog;h=refs/heads/misc> which is reviewable plz!
Comment 3 Jonny Lamb 2011-03-03 00:37:15 UTC
OOI, why didn't you use wocky_strdiff everywhere in your misc branch instead of any strcmps?
Comment 4 Jonny Lamb 2011-03-03 06:38:49 UTC
Your misc branch looks fine.

Your ack-and-nak branch also looks fine apart from:

 * the two TODOs in the tests
 * I got a bit confused by 7882c658b7d5. If you could highlight that get/set have exactly one child elements, result have zero or one child elements then that'd make things clearer. The way the RFC says how an error SHOULD have one and only one child element from the original IQ is confusing as I wondered how it could ever be >1.

Seems fine otherwise.
Comment 5 Will Thompson 2011-03-03 09:27:42 UTC
(In reply to comment #4)
> Your misc branch looks fine.

Merged, cheers.

> Your ack-and-nak branch also looks fine apart from:
> 
>  * the two TODOs in the tests

Fixed and squashed.

>  * I got a bit confused by 7882c658b7d5. If you could highlight that get/set
> have exactly one child elements, result have zero or one child elements then
> that'd make things clearer. The way the RFC says how an error SHOULD have one
> and only one child element from the original IQ is confusing as I wondered how
> it could ever be >1.

Fixed.
Comment 6 Will Thompson 2011-03-07 05:10:52 UTC
I've also added a patch adding wocky_porter_send_iq_gerror() for application-specific error conditions.
Comment 7 Jonny Lamb 2011-03-07 07:16:12 UTC
+  /* Send an illegal IQ with two child elements. We expect that
+   * wocky_porter_acknowledge_iq() should cope. */
+  iq = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
+      WOCKY_STANZA_SUB_TYPE_GET, "juliet@example.com", "romeo@example.net",
+      '(', "sup-dawg", ')',
+      '(', "i-heard-you-like-stanzas", ')',
+      NULL);
+  expected_reply = wocky_stanza_build (
+      WOCKY_STANZA_TYPE_IQ, WOCKY_STANZA_SUB_TYPE_RESULT,
+      "romeo@example.net", "juliet@example.com",
+      '(', "sup-dawg",
+        '@', "lions", "tigers",
+      ')',
+      NULL);

Isn't this a bit of a poor test? If you reply with something other than a 'sup-dawg' child element, then the result IQ has two child elements, right? It'd be cooler if this was more explicit.

Oh, no, I'm getting confused with error. Ignore that then.

LOOKS HOT.
Comment 8 Will Thompson 2011-03-10 02:52:15 UTC
Merged, thanks!


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.