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

[Zeeshan Ali]

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.

-- 
Regards,

Zeeshan Ali