Bug 26414

Summary:	Refactor and extend error parsing/production to support application-specific errors, etc.
Product:	Wocky	Reporter:	Will Thompson <will>
Component:	General	Assignee:	Will Thompson <will>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	medium	CC:	senko.rasic
Version:	git master	Keywords:	patch
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/wjt/wocky.git;a=shortlog;h=refs/heads/error-wrangling
Whiteboard:
i915 platform:		i915 features:

Description Will Thompson 2010-02-03 07:28:22 UTC

Currently Wocky has a hodge-podge of error parsing functions copied from Gabble. Some of them produce GErrors, but don't get the error text correctly; some produce a whole pile of pointers to nodes, strings and whatnot; etc. Also, the error node production functions only work with core stanza errors; if you want to produce, say, Jingle errors, you're out of luck.

My 'error-wrangling' branch adds the concept of specialized error domains, whereby you can register a GQuark (for the GError domain whose string happens to be the XMPP namespace) and a GEnum representing application-specific errors, along with which XMPP Core stanza errors they specialize. Then, it improves the error stanza production code to produce a full <error/> stanza from GErrors in those domains (or in the basic WOCKY_XMPP_ERROR domain, as supported before), and adds a function to break down a stanza error into the type of error, a GError for the core error, and a GError for the specialized error. (This split is needed so that you can get the core error if you don't understand the specialization.)

Then I went through and deleted all the superseded functions. It was a nice feeling.

== Outstanding points ==

Maybe we can't use the XML namespace as the GError quark. Consider the following case:

• A Wocky-based application implements a new XEP, with errors in the foo:bar namespace.
• Subsequently, Wocky includes that error domain itself, but enumerates the error codes in a different order.
• Now one of those orders of errors has to win, breaking either Wocky itself, or the application using it.

One way out of this trap would be to make wocky_xmpp_error_extract() take a list of specialized domains the calling code understands, and break up WockyXmppErrorDomain.domain into one quark for the GError domain, and another for the XML namespace.

Unrelatedly, some of the naming isn't great. I'm not overly fond of WockyXmppErrorDomain; perhaps WockyErrorNS? WockyXmppError should probably be WockyStanzaError or something. Finally, wocky_stanza_error_to_node() and wocky_xmpp_error_extract() are essentially inverses but have different-shaped names; I'm not sure which side of this fence to fall on. Suggestions?

Comment 1 Will Thompson 2010-02-03 08:27:55 UTC

(I have a corresponding Gabble branch that updates it for the changes this Wocky branch introduces.)

Comment 2 Sjoerd Simons 2010-02-12 08:40:52 UTC

d915abf0858f13c000203685bbfce8368803dd60:

The while loop doesn't actually iterate the list of children, causing an infinite loop if the error isn't the first child. Seems to be removed in a later commit

5c88062e9470be54720a5bca1a4a3276f028e578

First hunk has if (!quark), all others do if (quark == 0). 

4da39e6569b55ab29610e3f27c2de3ade4db5acd

Nitpick, i'm a sucker for user g_slist_next instead of ->next. But that's just me

86490fc3bcf9c87b0b7be9d6eaadadbc9cbf1c01

Nitpick, this assumes wocky_enum_from_nick doesn't change type_i iff it couldn't find the type. Might be more defensive to check the return and only then set it to TYPE_CANCEL

Comment 3 Will Thompson 2010-02-15 04:51:43 UTC

(In reply to comment #2)
> d915abf0858f13c000203685bbfce8368803dd60:
> 
> The while loop doesn't actually iterate the list of children, causing an
> infinite loop if the error isn't the first child. Seems to be removed in a
> later commit

Good catch. But it is indeed removed later. I could edit the past, but it doesn't seem too necessary.

> 5c88062e9470be54720a5bca1a4a3276f028e578
> 
> First hunk has if (!quark), all others do if (quark == 0).

Yep; fixed in 533bbdc.

> 4da39e6569b55ab29610e3f27c2de3ade4db5acd
> 
> Nitpick, i'm a sucker for user g_slist_next instead of ->next. But that's just
> me

I don't think it's any clearer, and it's more typing. :)

> 86490fc3bcf9c87b0b7be9d6eaadadbc9cbf1c01
> 
> Nitpick, this assumes wocky_enum_from_nick doesn't change type_i iff it
> couldn't find the type. Might be more defensive to check the return and only
> then set it to TYPE_CANCEL

Instead, I documented this assumption in enum_from_nick()'s docstring: f6842a6

Gabble branch on its way...

Comment 4 Will Thompson 2010-02-15 05:32:50 UTC

Here's a Gabble branch with minimal changes to work with a new snapshot of Wocky: http://git.collabora.co.uk/?p=user/wjt/telepathy-gabble-wjt.git;a=shortlog;h=refs/heads/error-wrangling

Comment 5 Will Thompson 2010-02-18 06:24:00 UTC

Both the Wocky branch and the Gabble branch are merged, with Sjoerd's approval! Will be in Gabble 0.9.5.

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.