[libvirt] [libvirt-glib] API to get/set custom metadata from/to domain config

[Zeeshan Ali (Khattak)]

On Mon, Jan 23, 2012 at 1:01 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Fri, Jan 20, 2012 at 11:11:57PM +0200, Zeeshan Ali (Khattak) wrote:
>> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
>>
>> ---
>>  libvirt-gconfig/libvirt-gconfig-domain.c  |  115 +++++++++++++++++++++++++++++
>>  libvirt-gconfig/libvirt-gconfig-domain.h  |    8 ++
>>  libvirt-gconfig/libvirt-gconfig-helpers.c |    4 +-
>>  libvirt-gconfig/libvirt-gconfig.sym       |    2 +
>>  4 files changed, 128 insertions(+), 1 deletions(-)
>>
>> diff --git a/libvirt-gconfig/libvirt-gconfig-domain.c b/libvirt-gconfig/libvirt-gconfig-domain.c
>> index 61af625..d1faabd 100644
>> --- a/libvirt-gconfig/libvirt-gconfig-domain.c
>> +++ b/libvirt-gconfig/libvirt-gconfig-domain.c
>> @@ -449,3 +449,118 @@ GList *gvir_config_domain_get_devices(GVirConfigDomain *domain)
>>
>>      return data.devices;
>>  }
>> +
>> +static void set_namespace_on_tree(xmlNodePtr node, xmlNsPtr namespace)
>> +{
>> +    xmlNodePtr child;
>> +
>> +    xmlSetNs(node, namespace);
>> +
>> +    for (child = node->children; child != NULL; child = child->next)
>> +        set_namespace_on_tree(child, namespace);
>
> I wouldn't make this recursive, I'd just set the namespace on the root
> node in case users of the API want to use different namespaces in the xml
> they embed in the domain.

Ah good point but OTOH we should ensure that no node in the XML is
using non/default namespace so maybe we should just set it for every
node that doesn't have a namespace already?

>> +    if (metadata_node == NULL)
>> +        return NULL;
>> +
>> +    for (node = metadata_node->children; node != NULL; node = node->next) {
>> +        if (node->ns != NULL &&
>> +            (g_strcmp0 ((gchar *)node->ns->href, ns_uri) == 0))
>> +            break;
>> +    }
>> +
>> +    return node;
>> +}
>
> Hmm seeing this loop, gvir_config_object_foreach_child could even be used
> here with a small helper struct returning the value.

Sure but IMHO code is simpler this way.

>> +    set_namespace_on_tree(node, namespace);
>> +    doc = node->doc;
>> +
>> +    old_node = gvir_config_domain_get_custom_xml_node(domain, ns_uri);
>> +    if (old_node != NULL)
>> +        xmlReplaceNode(old_node, node);
>> +    else {
>> +        xmlUnlinkNode(node);
>> +        xmlAddChild(metadata_node, node);
>> +    }
>
> This duplicates one of the GVirConfigObject helpers. I've tried to restrict
> xmlNodePtr use to libvirt-gconfig-object.c/libvirt-gconfig-helpers.c. So
> this patch which adds a lot of xmlNodePtr use to libvirt-gconfig-domain.c
> makes me a bit uncomfortable. Here I think we could use
> gvir_config_object_new_from_xml and reuse the existing helpers (with the
> addition of gvir_config_object_set_namespace).

But we are not create a GVirConfigObject here at all.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

P.S. Not replying to comments I agree with.