Hey, Thanks for reviewing all these patches! On Wed, Jan 04, 2012 at 10:34:15PM +0200, Zeeshan Ali (Khattak) wrote:
On Tue, Jan 3, 2012 at 4:50 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
When iterating over xmlNodePtr children to parse an XML document describing a libvirt configuration item, we want to ignore blank nodes that may have been added to make the initial XML document "more human readable". Since this will be useful in several places, move this code to a helper function.
Looks good, apart from the "more human readable" sarcasm. :)
It's not meant as a sarcasm, it's generally why some XML documents have some extra white space, so I'm simply stating a fact :)
Oh and a few small things:
+static gboolean add_one_feature(xmlNodePtr node, gpointer opaque)
'feature' is singular so 'one_' is pretty much redundant here.
This is an habit I have for callbacks used with _foreach functions, I put a "_one_" in the name to underline it's processing one element of the collection that is being iterated over. I removed it here, and did the same in the commit introducing _get_devices which has a add_one_device function.
+void gvir_config_xml_foreach_child(xmlNodePtr node, + GVirConfigXmlNodeIterator it_func, + gpointer opaque)
I'd suggest using 'iter_func' or just 'func' as the parameter name but this is entirely subjective so feel free to ignore this suggestion. :)
Changed
+ for (it = node->children; it != NULL; it = it->next) { + gboolean cont; + + if (xmlIsBlankNode(it)) + continue; + cont = it_func(it, opaque); + if (!cont) + break;
Since 'cont' is only used once and there is no big line (to not fit in the 120 cols limit), I suggest not using it here.
I don't like hiding function calls in if() conditions like if (!it_func(it, opaque)) break, that's why I used a variable, and I'd prefer to keep it that way :) Christophe