[virt-tools-list] [virt-manager PATCH 2/2] capabilities: detect ACPI and APIC capabilites properly

[Pavel Hrdina]

On Wed, Jul 08, 2015 at 12:54:11PM -0400, Cole Robinson wrote:
> On 07/08/2015 05:33 AM, Pavel Hrdina wrote:
> > Instead of hard-coding that ACPI and APIC are enabled by default, detect
> > their presence from libvirt capabilities and use it.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1215692
> > 
> > Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> > ---
> 
> Comments inline
> 
> > diff --git a/virtinst/capabilities.py b/virtinst/capabilities.py
> > index a26b4a2..553a430 100644
> > --- a/virtinst/capabilities.py
> > +++ b/virtinst/capabilities.py
> > @@ -206,6 +206,8 @@ class _CapsGuestFeatures(XMLBuilder):
> >      _XML_ROOT_NAME = "features"
> >  
> >      pae = XMLProperty("./pae", is_bool=True)
> > +    acpi = XMLProperty("./acpi/@default", is_onoff=True)
> > +    apic = XMLProperty("./apic/@default", is_onoff=True)
> >  
> >  
> >  class _CapsGuest(XMLBuilder):
> > @@ -298,6 +300,16 @@ class _CapsGuest(XMLBuilder):
> >              return True
> >          return False
> >  
> > +    def supports_acpi(self):
> > +        if self.features.acpi:
> > +            return True
> > +        return False
> > +
> > +    def supports_apic(self):
> > +        if self.features.apic:
> > +            return True
> > +        return False
> > +
> >  
> 
> You can just do: return bool(self.features.apic)
> 
> >  ############################
> >  # Main capabilities object #
> > diff --git a/virtinst/guest.py b/virtinst/guest.py
> > index 7ff7bd5..3f91ae9 100644
> > --- a/virtinst/guest.py
> > +++ b/virtinst/guest.py
> > @@ -840,9 +840,12 @@ class Guest(XMLBuilder):
> >              default = False
> >  
> >          if self.features.acpi == "default":
> > -            self.features.acpi = self._os_object.supports_acpi(default)
> > +            if default:
> > +                self.features.acpi = self.capsinfo.guest.supports_acpi()
> > +            else:
> > +                self.features.acpi = False
> >          if self.features.apic == "default":
> > -            self.features.apic = self._os_object.supports_apic(default)
> > +            self.features.apic = self.capsinfo.guest.supports_apic()
> >          if self.features.pae == "default":
> >              self.features.pae = self.capsinfo.guest.supports_pae()
> >  
> 
> You still need to abide the os_object bits. I'd say leave these 'if'
> conditionals alone, and set the 'default' variable above to also depend on
> capsinfo.guest.supports_*. But you'll probably need to have separate
> acpi_default and apic_default vars

Actually I've removed the functions from osdict, because the only function was,
that in case the host os is "msdos" don't use ACPI/APIC and in all other cases
set them as default.  I don't think, that this a correct detection.  ACPI/APIC
are HW features and it should not depend on the OS itself.

Pavel

> 
> Thanks,
> Cole
> 
> > diff --git a/virtinst/osdict.py b/virtinst/osdict.py
> > index 76c2260..bd82ae1 100644
> > --- a/virtinst/osdict.py
> > +++ b/virtinst/osdict.py
> > @@ -447,14 +447,6 @@ class _OsVariant(object):
> >      def supports_virtiommio(self):
> >          return self._is_related_to(["fedora19"])
> >  
> > -    def supports_acpi(self, default):
> > -        if self._family in ['msdos']:
> > -            return False
> > -        return default
> > -
> > -    def supports_apic(self, default):
> > -        return self.supports_acpi(default)
> > -
> >      def default_netmodel(self):
> >          """
> >          Default non-virtio net-model, since we check for that separately
> > 
>