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 | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libvirt-gconfig/libvirt-gconfig-domain-device.c b/libvirt-gconfig/libvirt-gconfig-domain-device.c index f78173a..2475519 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,22 @@ 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); + if (type == GVIR_CONFIG_TYPE_DOMAIN_DEVICE) + g_debug("Proper support for '%s' device nodes is not yet implemented", tree->name); + 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 Thu, Nov 03, 2016 at 10:06:22AM +0100, Zeeshan Ali wrote: > --- > tests/test-gconfig.c | 22 ++++++++++++++++++++++ > tests/xml/gconfig-domain-device-unknown.xml | 5 +++++ > 2 files changed, 27 insertions(+) > create mode 100644 tests/xml/gconfig-domain-device-unknown.xml > > 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)); > +} The whole point of this work is to make sure that the unknown nodes are kept when regenerating the XML document, so I would expect a call to check_xml() too after calling set_devices(domain, devices);

You should have g_list_free_full(devices, g_object_unref); when freeing the device list.

On Thu, Nov 03, 2016 at 02:05:46PM +0100, Zeeshan Ali wrote: > On Thu, Nov 3, 2016 at 1:03 PM, Christophe Fergeau <cfergeau(a)redhat.com&gt; wrote: > > On Thu, Nov 03, 2016 at 10:06:22AM +0100, Zeeshan Ali wrote: > >> --- > >> tests/test-gconfig.c | 22 ++++++++++++++++++++++ > >> tests/xml/gconfig-domain-device-unknown.xml | 5 +++++ > >> 2 files changed, 27 insertions(+) > >> create mode 100644 tests/xml/gconfig-domain-device-unknown.xml > >> > >> 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)); > >> +} > > > > The whole point of this work is to make sure that the unknown nodes are > > kept when regenerating the XML document, so I would expect a call to > > check_xml() too after calling set_devices(domain, devices); > > * g_assert_nonnull(devices); actually checks that the only dev in the > original xml is still there or not. It checks that a device is there, it does say nothing about how it's going to be serialized, so the test case could be working while what you actually need is broken. > * check_xml() does a simple string comparison and that fails since > set_devices() changes the format. Isn't it possible to use the exact same format as the output for the source XML?