Re: [libvirt] [glib 3/5] gconfig: Create objects for all domain device nodes

Hi Christophe, On Wed, Nov 2, 2016 at 6:21 PM, Christophe Fergeau <cfergeau(a)redhat.com&gt; 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.