[libvirt] [libvirt-glib 2/3] Add host capabilities API

[Zeeshan Ali (Khattak)]

On Wed, May 2, 2012 at 5:25 PM, Christophe Fergeau <cfergeau at 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