On Mon, Jan 23, 2012 at 1:01 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> On Fri, Jan 20, 2012 at 11:11:57PM +0200, Zeeshan Ali (Khattak) wrote:
>> From: "Zeeshan Ali (Khattak)" <zeeshanak@xxxxxxxxx>
>>
>> ---
>> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list