Re: [libvirt] [libvirt-glib 2/3] Add host capabilities API
On Wed, May 2, 2012 at 5:25 PM, Christophe Fergeau <cfergeau(a)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