On Thu, Nov 3, 2016 at 12:34 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list