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

[Cole Robinson]

On 03/13/2014 08:51 AM, 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>
> ---
>  tests/capabilities.py    | 15 ++++++++++-----
>  virtManager/details.py   |  3 ++-
>  virtinst/capabilities.py | 47 ++++++++++++++++++++++++++++++++++++++---------
>  virtinst/support.py      |  4 +++-
>  4 files changed, 53 insertions(+), 16 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..b137ede 100644
> --- a/virtinst/capabilities.py
> +++ b/virtinst/capabilities.py
> @@ -77,11 +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, conn):
> +        self._conn = conn
> +        self._cpus = None
> +

I'd recommend not storing 'conn' here, since it might cause some GC issue:
VirtinstConnection holds a reference to capabilities which may hold a
reference to connection. Just change get_cpus to take the conn explicitly.

> +    def get_cpus(self, arch):
> +        if self._cpus is not None:
> +            return self._cpus
> +
> +        if self._conn and \
> +           self._conn.check_support(self._conn.SUPPORT_CONN_CPU_MODEL_NAMES):
> +            self._cpus = [CPUValuesModel(i) for i in
> +                          self._conn.libvirtconn.getCPUModelNames(arch, 0)]
> +            return self._cpus
> +
> +        return None

Style nit: please drop the \ escape and just wrap the whole thing in parenths.

> +class CPUMapFileValues(object):
> +    """
> +    Fallback method to lists cpu models, parsed directly from libvirt's local
> +    cpu_map.xml
> +    """
> +    def __init__(self, conn=None, cpu_filename=None):
>          self.archmap = {}
> +        ignore = conn
>          if not cpu_filename:
>              cpu_filename = "/usr/share/libvirt/cpu_map.xml"

Looks like passing in cpu_filename in __init__ isn't even used, so you can
drop that. Then just have CPUMapFileValues subclass CPUValues so pylint will
give us warnings if the API diverges.

- Cole