On Wed, May 2, 2012 at 5:25 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list