[virt-tools-list] [PATCH v2 virt-manager] virtinst: use libvirt getCPUModelNames when available

[Cole Robinson]

On 03/13/2014 01:15 PM, Giuseppe Scrivano wrote:
> Read the list of CPU models trough getCPUModelNames instead of
> accessing directly the file cpu_map.xml.
> 
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1060316
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan at redhat.com>
> ---
> thanks for the comments.
> 
> What about this version?
> 
>  tests/capabilities.py    | 15 ++++++++++-----
>  virtManager/details.py   |  3 ++-
>  virtinst/capabilities.py | 50 +++++++++++++++++++++++++++++++++++++-----------
>  virtinst/support.py      |  4 +++-
>  4 files changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/tests/capabilities.py b/tests/capabilities.py
> index c062e4c..db69c6f 100644
> --- a/tests/capabilities.py
> +++ b/tests/capabilities.py
> @@ -18,6 +18,7 @@
>  import os
>  import unittest
>  
> +from tests import utils
>  from virtinst import CapabilitiesParser as capabilities
>  
>  
> @@ -221,13 +222,12 @@ class TestCapabilities(unittest.TestCase):
>  
>      def testCPUMap(self):
>          caps = self._buildCaps("libvirt-0.7.6-qemu-caps.xml")
> -        cpu_64 = caps.get_cpu_values("x86_64")
> -        cpu_32 = caps.get_cpu_values("i486")
> -        cpu_random = caps.get_cpu_values("mips")
> +        cpu_64 = caps.get_cpu_values(None, "x86_64")
> +        cpu_32 = caps.get_cpu_values(None, "i486")
> +        cpu_random = caps.get_cpu_values(None, "mips")
>  
>          def test_cpu_map(cpumap, cpus):
> -            cpunames = sorted([c.model for c in cpumap.cpus],
> -                              key=str.lower)
> +            cpunames = sorted([c.model for c in cpumap], key=str.lower)
>  
>              for c in cpus:
>                  self.assertTrue(c in cpunames)
> @@ -243,5 +243,10 @@ class TestCapabilities(unittest.TestCase):
>          test_cpu_map(cpu_64, x86_cpunames)
>          test_cpu_map(cpu_random, [])
>  
> +        conn = utils.open_testdriver()
> +        cpu_64 = caps.get_cpu_values(conn, "x86_64")
> +        self.assertTrue(len(cpu_64) > 0)
> +
> +
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/virtManager/details.py b/virtManager/details.py
> index 56a0d60..14c77fe 100644
> --- a/virtManager/details.py
> +++ b/virtManager/details.py
> @@ -975,7 +975,8 @@ class vmmDetails(vmmGObjectUI):
>          no_default = not self.is_customize_dialog
>  
>          try:
> -            cpu_names = caps.get_cpu_values(self.vm.get_arch()).cpus
> +            cpu_names = caps.get_cpu_values(self.conn.get_backend(),
> +                                            self.vm.get_arch())
>          except:
>              cpu_names = []
>              logging.exception("Error populating CPU model list")
> diff --git a/virtinst/capabilities.py b/virtinst/capabilities.py
> index 2ab39cd..0bedd65 100644
> --- a/virtinst/capabilities.py
> +++ b/virtinst/capabilities.py
> @@ -77,13 +77,33 @@ class CPUValuesArch(object):
>  
>  class CPUValues(object):
>      """
> -    Lists valid values for domain <cpu> parameters, parsed from libvirt's
> -    local cpu_map.xml
> +    Lists valid values for cpu models obtained trough libvirt's getCPUModelNames
>      """
> -    def __init__(self, cpu_filename=None):
> +    def __init__(self):
> +        self._cpus = None
> +
> +    def get_cpus(self, arch, conn):
> +        if self._cpus is not None:
> +            return self._cpus
> +
> +        if (conn and
> +            conn.check_support(conn.SUPPORT_CONN_CPU_MODEL_NAMES)):
> +            self._cpus = [CPUValuesModel(i) for i in
> +                          conn.libvirtconn.getCPUModelNames(arch, 0)]
> +            return self._cpus
> +
> +        return None

I think 'return None' should be 'return []', since it might affect
virt-manager's use of get_cpu_values

> +
> +
> +class CPUMapFileValues(CPUValues):
> +    """
> +    Fallback method to lists cpu models, parsed directly from libvirt's local
> +    cpu_map.xml
> +    """
> +    def __init__(self):
> +        CPUValues.__init__(self)
>          self.archmap = {}
> -        if not cpu_filename:
> -            cpu_filename = "/usr/share/libvirt/cpu_map.xml"
> +        cpu_filename = "/usr/share/libvirt/cpu_map.xml"
>          xml = file(cpu_filename).read()
>  
>          util.parse_node_helper(xml, "cpus",
> @@ -99,7 +119,8 @@ class CPUValues(object):
>  
>              child = child.next
>  
> -    def get_arch(self, arch):
> +    def get_cpus(self, arch, conn):
> +        ignore = conn
>          if not arch:
>              return None
>          if re.match(r'i[4-9]86', arch):
> @@ -112,7 +133,7 @@ class CPUValues(object):
>              cpumap = CPUValuesArch(arch)
>              self.archmap[arch] = cpumap
>  
> -        return cpumap
> +        return cpumap.cpus
>  
>  
>  class Features(object):
> @@ -595,12 +616,19 @@ class Capabilities(object):
>                  self.guests.append(Guest(child))
>              child = child.next
>  
> -    def get_cpu_values(self, arch):
> -        if not self._cpu_values:
> -            self._cpu_values = CPUValues()
> +    def get_cpu_values(self, conn, arch):
> +        if self._cpu_values:
> +            return self._cpu_values.get_cpus(arch, conn)
>  
> -        return self._cpu_values.get_arch(arch)
> +        # Iterate over the available methods until a set of CPU models is found
> +        for mode in (CPUValues, CPUMapFileValues):
> +            cpu_values = mode()
> +            cpus = cpu_values.get_cpus(arch, conn)
> +            if cpus:
> +                self._cpu_values = cpu_values
> +                return cpus
>  
> +        return None
> 

Same here.

ACK with that fix, feel free to just push it without reposting.

- Cole