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

[Christophe Fergeau]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161103/7b751e1d/attachment-0001.sig>