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

[Zeeshan Ali]

On Thu, Nov 3, 2016 at 12:34 PM, Christophe Fergeau <cfergeau at redhat.com> 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 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.

True. But the new patch adds a debug for both cases. Not sure we need
to differentiate between the two cases.

-- 
Regards,

Zeeshan Ali