7:29 a.m.
On Thu, Nov 3, 2016 at 12:34 PM, Christophe Fergeau <cfergeau(a)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(a)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