On Wed, May 2, 2012 at 5:25 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Tue, May 01, 2012 at 08:30:39PM +0300, Zeeshan Ali (Khattak) wrote:
+{ + g_debug("Init GVirConfigCapabilitiesCPUFeature=%p", conn); + + conn->priv = GVIR_CONFIG_CAPABILITIES_CPU_FEATURE_GET_PRIVATE(conn); +} + +G_GNUC_INTERNAL GVirConfigCapabilitiesCPUFeature * +gvir_config_capabilities_cpu_feature_new_from_tree(GVirConfigXmlDoc *doc, + xmlNodePtr tree) +{ + GVirConfigObject *object; + + object = gvir_config_object_new_from_tree(GVIR_CONFIG_TYPE_CAPABILITIES_CPU_FEATURE, + doc, + NULL, + tree); + + return GVIR_CONFIG_CAPABILITIES_CPU_FEATURE(object); +} + +const gchar * +gvir_config_capabilities_cpu_feature_get_name(GVirConfigCapabilitiesCPUFeature *caps) +{ + xmlNodePtr node; + + node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(caps)); + + if (g_strcmp0((const gchar *)node->name, "feature") == 0) + return gvir_config_xml_get_attribute_content(node, "name"); + else + return (const gchar *)node->name;
This "if" is needed because we can have <features><foo/></features> VS <feature name="foo">? This deserves a comment explaining why we do this test.
Yup!
Having a per-feature GVirConfigObject seems overkill since it will only be a string wrapper, and a GVirConfigObject wrapping just a string with no node name identifying the type of the node is unusual.
Thats only because I haven't added 2 possible getters of this object. We don't need them right now but they could be added when needed later. I have discussed this with Daniel and he and I both think this 'feature' deserves a separate class.
+ +struct GetFeatureData { + GVirConfigXmlDoc *doc; + GList *features; +}; + +static gboolean add_feature(xmlNodePtr node, gpointer opaque) +{ + struct GetFeatureData* data = (struct GetFeatureData*)opaque; + GVirConfigCapabilitiesCPUFeature *feature; + + if (g_strcmp0((const gchar *)node->name, "feature") != 0) + return TRUE;
Is it expected that "features" nodes are ignored?
? Its the other way around: We ignore other nodes and create objects for 'feature' nodes.
Are the 2 kind of nodes (feature/features) two different things that we want to expose differently in the API?
I don't think we need separate classes for both. They both represent the same concept, just that libvirt capabilties xml is a bit inconsistent AFAICT.
+ node = gvir_config_xml_get_element(node, "cpu", NULL); + g_return_val_if_fail(node != NULL, NULL);
Is it a mandatory element in the capabilities XML?
According to RNG, it is mandatory.
+GVirConfigCapabilitiesHost * +gvir_config_capabilities_get_host(GVirConfigCapabilities *caps) +{ + GVirConfigXmlDoc *doc; + xmlNodePtr node; + + g_return_val_if_fail(GVIR_CONFIG_IS_CAPABILITIES(caps), NULL); + + g_object_get(G_OBJECT(caps), "doc", &doc, NULL); + + node = gvir_config_object_get_xml_node(GVIR_CONFIG_OBJECT(caps)); + g_return_val_if_fail(node != NULL, NULL); + + node = gvir_config_xml_get_element(node, "host", NULL); + g_return_val_if_fail(node != NULL, NULL); + + return gvir_config_capabilities_host_new_from_tree(doc, node); +}
This is the same code as gvir_config_capabilities_host_get_cpu, if this pattern gets repeated often, it will be worth trying to factor it in a helper function.
True and since we'll need to use g_object_new() instead in that helper, we can just ditch all these new private headers. -- Regards, Zeeshan Ali (Khattak) FSF member#5124