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. Christophe
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list