[libvirt] [glib 1/5] gconfig: Allow schema to be NULL
Validation (if attempted) should just fail in this case instead of crashing. --- libvirt-gconfig/libvirt-gconfig-object.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libvirt-gconfig/libvirt-gconfig-object.c b/libvirt-gconfig/libvirt-gconfig-object.c index 6225de2..851e35c 100644 --- a/libvirt-gconfig/libvirt-gconfig-object.c +++ b/libvirt-gconfig/libvirt-gconfig-object.c @@ -209,6 +209,14 @@ void gvir_config_object_validate(GVirConfigObject *config, return; } + if (!priv->schema) { + gvir_config_set_error_literal(err, + GVIR_CONFIG_OBJECT_ERROR, + 0, + _("No XML schema associated with this config object")); + return; + } + rngParser = xmlRelaxNGNewParserCtxt(priv->schema); if (!rngParser) { gvir_config_set_error(err, -- 2.9.3
We'll need to instatiate DomainDevice baseclass itself for unknown (think new devices added to libvirt XML) devices in a following patch. This change makes that possible. This doesn't break any API or ABI to the best of my knowledge and this assumption was confirmed by Emmanuele Bassi and Tim-Philipp Muller. --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index 8a75cea..f78173a 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -33,7 +33,7 @@ struct _GVirConfigDomainDevicePrivate gboolean unused; }; -G_DEFINE_ABSTRACT_TYPE(GVirConfigDomainDevice, gvir_config_domain_device, GVIR_CONFIG_TYPE_OBJECT); +G_DEFINE_TYPE(GVirConfigDomainDevice, gvir_config_domain_device, GVIR_CONFIG_TYPE_OBJECT); static void gvir_config_domain_device_class_init(GVirConfigDomainDeviceClass *klass) -- 2.9.3
On Tue, Nov 01, 2016 at 05:46:04PM +0100, Zeeshan Ali wrote:
We'll need to instatiate DomainDevice baseclass itself for unknown (think new devices added to libvirt XML) devices in a following patch. This change makes that possible.
This doesn't break any API or ABI to the best of my knowledge and this assumption was confirmed by Emmanuele Bassi and Tim-Philipp Muller.
G_DEFINE_ABSTRACT_TYPE expands to G_DEFINE_TYPE_EXTENDED (TN, t_n, T_P, G_TYPE_FLAG_ABSTRACT, {}) while G_DEFINE_TYPE expands to G_DEFINE_TYPE_EXTENDED (TN, t_n, T_P, 0, {}) so the only change is setting G_TYPE_FLAG_ABSTRACT, which is not an ABI/API break. So agree that we're safe.
--- libvirt-gconfig/libvirt-gconfig-domain-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index 8a75cea..f78173a 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -33,7 +33,7 @@ struct _GVirConfigDomainDevicePrivate gboolean unused; };
-G_DEFINE_ABSTRACT_TYPE(GVirConfigDomainDevice, gvir_config_domain_device, GVIR_CONFIG_TYPE_OBJECT); +G_DEFINE_TYPE(GVirConfigDomainDevice, gvir_config_domain_device, GVIR_CONFIG_TYPE_OBJECT);
static void gvir_config_domain_device_class_init(GVirConfigDomainDeviceClass *klass)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Currently we can and do get into serious trouble with this kind of code: devices = gvir_config_domain_get_devices(domain); gvir_config_domain_set_devices(domain, domain); since the first call above won't return a complete list of objects present in the domain but only the ones we have specific classes for and the second call above overwrites all device nodes under the domain. This lately made Boxes break against the latest libvirt, where a new device node was made compulsory[1]. Although we should add support for all know domain devices ASAP, new devices will be added in future and this can happen again. So let's first ensure that gvir_config_domain_get_devices() always returns all devices under the domain. All unknown/unimplemented devices will now be returned as the very generic DomainDevice objects. Once we add support for a particular device, there will be no API/ABI breakage since the new class will inherit from DomainDevice class. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1388091 --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index f78173a..3efca62 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -64,7 +64,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"controller")) { return gvir_config_domain_controller_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"lease")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"hostdev")) { return gvir_config_domain_hostdev_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"redirdev")) { @@ -76,7 +76,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"input")) { type = GVIR_CONFIG_TYPE_DOMAIN_INPUT; } else if (xmlStrEqual(tree->name, (xmlChar*)"hub")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"graphics")) { return gvir_config_domain_graphics_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"video")) { @@ -90,22 +90,19 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"channel")) { type = GVIR_CONFIG_TYPE_DOMAIN_CHANNEL; } else if (xmlStrEqual(tree->name, (xmlChar*)"watchdog")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"sound")) { type = GVIR_CONFIG_TYPE_DOMAIN_SOUND; } else if (xmlStrEqual(tree->name, (xmlChar*)"memballoon")) { type = GVIR_CONFIG_TYPE_DOMAIN_MEMBALLOON; } else { g_debug("Unknown device node: %s", tree->name); - return NULL; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } g_return_val_if_fail(g_type_is_a(type, GVIR_CONFIG_TYPE_DOMAIN_DEVICE), NULL); return GVIR_CONFIG_DOMAIN_DEVICE(gvir_config_object_new_from_tree(type, doc, NULL, tree)); -unimplemented: - g_debug("Parsing of '%s' device nodes is unimplemented", tree->name); - return NULL; } -- 2.9.3
On Tue, Nov 01, 2016 at 05:46:05PM +0100, Zeeshan Ali wrote:
Currently we can and do get into serious trouble with this kind of code:
devices = gvir_config_domain_get_devices(domain); gvir_config_domain_set_devices(domain, domain);
since the first call above won't return a complete list of objects present in the domain but only the ones we have specific classes for and the second call above overwrites all device nodes under the domain. This lately made Boxes break against the latest libvirt, where a new device node was made compulsory[1].
Although we should add support for all know domain devices ASAP, new devices will be added in future and this can happen again. So let's first ensure that gvir_config_domain_get_devices() always returns all devices under the domain. All unknown/unimplemented devices will now be returned as the very generic DomainDevice objects. Once we add support for a particular device, there will be no API/ABI breakage since the new class will inherit from DomainDevice class.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1388091 --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Hey, On Tue, Nov 01, 2016 at 05:46:05PM +0100, Zeeshan Ali wrote:
Currently we can and do get into serious trouble with this kind of code:
devices = gvir_config_domain_get_devices(domain); gvir_config_domain_set_devices(domain, domain);
since the first call above won't return a complete list of objects present in the domain but only the ones we have specific classes for and the second call above overwrites all device nodes under the domain. This lately made Boxes break against the latest libvirt, where a new device node was made compulsory[1].
Although we should add support for all know domain devices ASAP, new devices will be added in future and this can happen again. So let's first ensure that gvir_config_domain_get_devices() always returns all devices under the domain. All unknown/unimplemented devices will now be returned as the very generic DomainDevice objects. Once we add support for a particular device, there will be no API/ABI breakage since the new class will inherit from DomainDevice class.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1388091 --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index f78173a..3efca62 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -64,7 +64,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"controller")) { return gvir_config_domain_controller_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"lease")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"hostdev")) { return gvir_config_domain_hostdev_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"redirdev")) { @@ -76,7 +76,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"input")) { type = GVIR_CONFIG_TYPE_DOMAIN_INPUT; } else if (xmlStrEqual(tree->name, (xmlChar*)"hub")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"graphics")) { return gvir_config_domain_graphics_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"video")) { @@ -90,22 +90,19 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"channel")) { type = GVIR_CONFIG_TYPE_DOMAIN_CHANNEL; } else if (xmlStrEqual(tree->name, (xmlChar*)"watchdog")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"sound")) { type = GVIR_CONFIG_TYPE_DOMAIN_SOUND; } else if (xmlStrEqual(tree->name, (xmlChar*)"memballoon")) { type = GVIR_CONFIG_TYPE_DOMAIN_MEMBALLOON; } else { g_debug("Unknown device node: %s", tree->name); - return NULL; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; }
g_return_val_if_fail(g_type_is_a(type, GVIR_CONFIG_TYPE_DOMAIN_DEVICE), NULL);
return GVIR_CONFIG_DOMAIN_DEVICE(gvir_config_object_new_from_tree(type, doc, NULL, tree)); -unimplemented: - g_debug("Parsing of '%s' device nodes is unimplemented", tree->name);
Very minor, but I would have kept this g_debug(). Christophe
Hi Christophe, On Wed, Nov 2, 2016 at 6:21 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
Hey,
On Tue, Nov 01, 2016 at 05:46:05PM +0100, Zeeshan Ali wrote:
Currently we can and do get into serious trouble with this kind of code:
devices = gvir_config_domain_get_devices(domain); gvir_config_domain_set_devices(domain, domain);
since the first call above won't return a complete list of objects present in the domain but only the ones we have specific classes for and the second call above overwrites all device nodes under the domain. This lately made Boxes break against the latest libvirt, where a new device node was made compulsory[1].
Although we should add support for all know domain devices ASAP, new devices will be added in future and this can happen again. So let's first ensure that gvir_config_domain_get_devices() always returns all devices under the domain. All unknown/unimplemented devices will now be returned as the very generic DomainDevice objects. Once we add support for a particular device, there will be no API/ABI breakage since the new class will inherit from DomainDevice class.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1388091 --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index f78173a..3efca62 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -64,7 +64,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"controller")) { return gvir_config_domain_controller_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"lease")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"hostdev")) { return gvir_config_domain_hostdev_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"redirdev")) { @@ -76,7 +76,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"input")) { type = GVIR_CONFIG_TYPE_DOMAIN_INPUT; } else if (xmlStrEqual(tree->name, (xmlChar*)"hub")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"graphics")) { return gvir_config_domain_graphics_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"video")) { @@ -90,22 +90,19 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"channel")) { type = GVIR_CONFIG_TYPE_DOMAIN_CHANNEL; } else if (xmlStrEqual(tree->name, (xmlChar*)"watchdog")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"sound")) { type = GVIR_CONFIG_TYPE_DOMAIN_SOUND; } else if (xmlStrEqual(tree->name, (xmlChar*)"memballoon")) { type = GVIR_CONFIG_TYPE_DOMAIN_MEMBALLOON; } else { g_debug("Unknown device node: %s", tree->name); - return NULL; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; }
g_return_val_if_fail(g_type_is_a(type, GVIR_CONFIG_TYPE_DOMAIN_DEVICE), NULL);
return GVIR_CONFIG_DOMAIN_DEVICE(gvir_config_object_new_from_tree(type, doc, NULL, tree)); -unimplemented: - g_debug("Parsing of '%s' device nodes is unimplemented", tree->name);
Very minor, but I would have kept this g_debug().
Sure but I think it should be used for even unknown cases, not just known unimplemented cases. -- Regards, Zeeshan Ali
On Thu, Nov 03, 2016 at 10:00:56AM +0100, Zeeshan Ali wrote:
Hi Christophe,
On Wed, Nov 2, 2016 at 6:21 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
Hey,
On Tue, Nov 01, 2016 at 05:46:05PM +0100, Zeeshan Ali wrote:
Currently we can and do get into serious trouble with this kind of code:
devices = gvir_config_domain_get_devices(domain); gvir_config_domain_set_devices(domain, domain);
since the first call above won't return a complete list of objects present in the domain but only the ones we have specific classes for and the second call above overwrites all device nodes under the domain. This lately made Boxes break against the latest libvirt, where a new device node was made compulsory[1].
Although we should add support for all know domain devices ASAP, new devices will be added in future and this can happen again. So let's first ensure that gvir_config_domain_get_devices() always returns all devices under the domain. All unknown/unimplemented devices will now be returned as the very generic DomainDevice objects. Once we add support for a particular device, there will be no API/ABI breakage since the new class will inherit from DomainDevice class.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1388091 --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index f78173a..3efca62 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -64,7 +64,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"controller")) { return gvir_config_domain_controller_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"lease")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"hostdev")) { return gvir_config_domain_hostdev_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"redirdev")) { @@ -76,7 +76,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"input")) { type = GVIR_CONFIG_TYPE_DOMAIN_INPUT; } else if (xmlStrEqual(tree->name, (xmlChar*)"hub")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"graphics")) { return gvir_config_domain_graphics_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"video")) { @@ -90,22 +90,19 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"channel")) { type = GVIR_CONFIG_TYPE_DOMAIN_CHANNEL; } else if (xmlStrEqual(tree->name, (xmlChar*)"watchdog")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"sound")) { type = GVIR_CONFIG_TYPE_DOMAIN_SOUND; } else if (xmlStrEqual(tree->name, (xmlChar*)"memballoon")) { type = GVIR_CONFIG_TYPE_DOMAIN_MEMBALLOON; } else { g_debug("Unknown device node: %s", tree->name); - return NULL; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; }
g_return_val_if_fail(g_type_is_a(type, GVIR_CONFIG_TYPE_DOMAIN_DEVICE), NULL);
return GVIR_CONFIG_DOMAIN_DEVICE(gvir_config_object_new_from_tree(type, doc, NULL, tree)); -unimplemented: - g_debug("Parsing of '%s' device nodes is unimplemented", tree->name);
Very minor, but I would have kept this g_debug().
Sure but I think it should be used for even unknown cases, not just known unimplemented cases.
There is already a g_debug("Unknown device node: %s", tree->name); when the node is unknown, I don't see a g_debug() anymore in the known unimplemented cases. Christophe
On Thu, Nov 3, 2016 at 12:34 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Thu, Nov 03, 2016 at 10:00:56AM +0100, Zeeshan Ali wrote:
Hi Christophe,
On Wed, Nov 2, 2016 at 6:21 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
Hey,
On Tue, Nov 01, 2016 at 05:46:05PM +0100, Zeeshan Ali wrote:
Currently we can and do get into serious trouble with this kind of code:
devices = gvir_config_domain_get_devices(domain); gvir_config_domain_set_devices(domain, domain);
since the first call above won't return a complete list of objects present in the domain but only the ones we have specific classes for and the second call above overwrites all device nodes under the domain. This lately made Boxes break against the latest libvirt, where a new device node was made compulsory[1].
Although we should add support for all know domain devices ASAP, new devices will be added in future and this can happen again. So let's first ensure that gvir_config_domain_get_devices() always returns all devices under the domain. All unknown/unimplemented devices will now be returned as the very generic DomainDevice objects. Once we add support for a particular device, there will be no API/ABI breakage since the new class will inherit from DomainDevice class.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1388091 --- libvirt-gconfig/libvirt-gconfig-domain-device.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index f78173a..3efca62 100644 --- a/libvirt-gconfig/libvirt-gconfig-domain-device.c +++ b/libvirt-gconfig/libvirt-gconfig-domain-device.c @@ -64,7 +64,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"controller")) { return gvir_config_domain_controller_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"lease")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"hostdev")) { return gvir_config_domain_hostdev_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"redirdev")) { @@ -76,7 +76,7 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"input")) { type = GVIR_CONFIG_TYPE_DOMAIN_INPUT; } else if (xmlStrEqual(tree->name, (xmlChar*)"hub")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"graphics")) { return gvir_config_domain_graphics_new_from_tree(doc, tree); } else if (xmlStrEqual(tree->name, (xmlChar*)"video")) { @@ -90,22 +90,19 @@ gvir_config_domain_device_new_from_tree(GVirConfigXmlDoc *doc, } else if (xmlStrEqual(tree->name, (xmlChar*)"channel")) { type = GVIR_CONFIG_TYPE_DOMAIN_CHANNEL; } else if (xmlStrEqual(tree->name, (xmlChar*)"watchdog")) { - goto unimplemented; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; } else if (xmlStrEqual(tree->name, (xmlChar*)"sound")) { type = GVIR_CONFIG_TYPE_DOMAIN_SOUND; } else if (xmlStrEqual(tree->name, (xmlChar*)"memballoon")) { type = GVIR_CONFIG_TYPE_DOMAIN_MEMBALLOON; } else { g_debug("Unknown device node: %s", tree->name); - return NULL; + type = GVIR_CONFIG_TYPE_DOMAIN_DEVICE; }
g_return_val_if_fail(g_type_is_a(type, GVIR_CONFIG_TYPE_DOMAIN_DEVICE), NULL);
return GVIR_CONFIG_DOMAIN_DEVICE(gvir_config_object_new_from_tree(type, doc, NULL, tree)); -unimplemented: - g_debug("Parsing of '%s' device nodes is unimplemented", tree->name);
Very minor, but I would have kept this g_debug().
Sure but I think it should be used for even unknown cases, not just known unimplemented cases.
There is already a g_debug("Unknown device node: %s", tree->name); when the node is unknown, I don't see a g_debug() anymore in the known unimplemented cases.
True. But the new patch adds a debug for both cases. Not sure we need to differentiate between the two cases. -- Regards, Zeeshan Ali
We'll need to load XML from file in another function, that will be added in a following patch. --- tests/test-gconfig.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/test-gconfig.c b/tests/test-gconfig.c index a26bb5f..5389a26 100644 --- a/tests/test-gconfig.c +++ b/tests/test-gconfig.c @@ -30,12 +30,11 @@ g_free(alloced_str); \ } G_STMT_END -static void check_xml(GVirConfigDomain *domain, const char *reference_file) +static char * load_xml(const char *reference_file) { const char *reference_path; GError *error = NULL; char *reference_xml; - char *xml; reference_path = g_test_get_filename(G_TEST_DIST, "xml", reference_file, NULL); @@ -45,6 +44,18 @@ static void check_xml(GVirConfigDomain *domain, const char *reference_file) * gedit, workaround this issue by removing trailing whitespace from * the reference file */ g_strchomp(reference_xml); + + return reference_xml; +} + + +static void check_xml(GVirConfigDomain *domain, const char *reference_file) +{ + char *reference_xml; + char *xml; + + reference_xml = load_xml(reference_file); + xml = gvir_config_object_to_xml(GVIR_CONFIG_OBJECT(domain)); g_assert_cmpstr(xml, ==, reference_xml); g_free(xml); -- 2.9.3
On Tue, Nov 01, 2016 at 05:46:06PM +0100, Zeeshan Ali wrote:
We'll need to load XML from file in another function, that will be added in a following patch. --- tests/test-gconfig.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
--- tests/test-gconfig.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test-gconfig.c b/tests/test-gconfig.c index 5389a26..b91f5af 100644 --- a/tests/test-gconfig.c +++ b/tests/test-gconfig.c @@ -762,6 +762,26 @@ static void test_domain_device_pci_hostdev(void) g_object_unref(G_OBJECT(domain)); } +static void test_domain_device_unknown(void) +{ + GVirConfigDomain *domain; + GList *devices; + GError *error = NULL; + char *xml; + + xml = load_xml("gconfig-domain-device-unknown.xml"); + + domain = gvir_config_domain_new_from_xml(xml, &error); + g_assert_no_error(error); + + devices = gvir_config_domain_get_devices(domain); + g_assert_nonnull(devices); + + g_list_free(devices); + g_object_unref(G_OBJECT(domain)); +} + + int main(int argc, char **argv) { gvir_config_init(&argc, &argv); @@ -793,6 +813,8 @@ int main(int argc, char **argv) test_domain_device_usb_redir); g_test_add_func("/libvirt-gconfig/domain-device-pci-hostdev", test_domain_device_pci_hostdev); + g_test_add_func("/libvirt-gconfig/domain-device-unknown", + test_domain_device_unknown); return g_test_run(); } -- 2.9.3
On Tue, Nov 01, 2016 at 05:46:07PM +0100, Zeeshan Ali wrote:
--- tests/test-gconfig.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/tests/test-gconfig.c b/tests/test-gconfig.c index 5389a26..b91f5af 100644 --- a/tests/test-gconfig.c +++ b/tests/test-gconfig.c @@ -762,6 +762,26 @@ static void test_domain_device_pci_hostdev(void) g_object_unref(G_OBJECT(domain)); }
+static void test_domain_device_unknown(void) +{ + GVirConfigDomain *domain; + GList *devices; + GError *error = NULL; + char *xml; + + xml = load_xml("gconfig-domain-device-unknown.xml");
I think you forgot to 'git add' this new XML file.
+ + domain = gvir_config_domain_new_from_xml(xml, &error); + g_assert_no_error(error); + + devices = gvir_config_domain_get_devices(domain); + g_assert_nonnull(devices); + + g_list_free(devices); + g_object_unref(G_OBJECT(domain)); +} + + int main(int argc, char **argv) { gvir_config_init(&argc, &argv); @@ -793,6 +813,8 @@ int main(int argc, char **argv) test_domain_device_usb_redir); g_test_add_func("/libvirt-gconfig/domain-device-pci-hostdev", test_domain_device_pci_hostdev); + g_test_add_func("/libvirt-gconfig/domain-device-unknown", + test_domain_device_unknown);
return g_test_run(); }
ACK if you fix the missing file when pushing. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Tue, Nov 01, 2016 at 05:46:03PM +0100, Zeeshan Ali wrote:
Validation (if attempted) should just fail in this case instead of crashing. --- libvirt-gconfig/libvirt-gconfig-object.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/libvirt-gconfig/libvirt-gconfig-object.c b/libvirt-gconfig/libvirt-gconfig-object.c index 6225de2..851e35c 100644 --- a/libvirt-gconfig/libvirt-gconfig-object.c +++ b/libvirt-gconfig/libvirt-gconfig-object.c @@ -209,6 +209,14 @@ void gvir_config_object_validate(GVirConfigObject *config, return; }
+ if (!priv->schema) { + gvir_config_set_error_literal(err, + GVIR_CONFIG_OBJECT_ERROR, + 0, + _("No XML schema associated with this config object")); + return; + } + rngParser = xmlRelaxNGNewParserCtxt(priv->schema); if (!rngParser) { gvir_config_set_error(err, --
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Zeeshan Ali