Change in vdsm[master]: caps/lib: move CPU architecture details to lib/cpuarch

nsoffer at redhat.com nsoffer at redhat.com
Mon Dec 14 00:08:06 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: caps/lib: move CPU architecture details to lib/cpuarch
......................................................................


Patch Set 2: Code-Review-1

(8 comments)

Partial review.

https://gerrit.ovirt.org/#/c/49972/2/lib/vdsm/cpuarch.py
File lib/vdsm/cpuarch.py:

Line 34:     def __init__(self, target_arch):
Line 35:         self._target_arch = target_arch
Line 36: 
Line 37:     def __str__(self):
Line 38:         return '{}'.format(self._target_arch)
This is not very helpful message. Is this part of vdsm api, or internal error?
Line 39: 
Line 40: 
Line 41: def arch_to_cpu_map(target_arch=None):
Line 42:     '''


Line 37:     def __str__(self):
Line 38:         return '{}'.format(self._target_arch)
Line 39: 
Line 40: 
Line 41: def arch_to_cpu_map(target_arch=None):
cpu_map is not good name, since this returns a single value, not a map.

Maybe cpu_arch_to_libvirt_cpu_arch()?
Line 42:     '''
Line 43:     Convert the CPU architecture to a libvirt's cpu_map representation.
Line 44: 
Line 45:     Arguments:


Line 45:     Arguments:
Line 46: 
Line 47:     target_arch Optional. Accepts a string of one of the supported
Line 48:                 architectures. If not supplied, the machine's architecture
Line 49:                 is used.
Why do we need default value for this? It only make this function more confusing. I would simplify and make arch required.
Line 50: 
Line 51:     Returns:
Line 52: 
Line 53:     A string representation of the architecture in libvirt's cpu_map file


Line 67: 
Line 68:     return _architecture_to_cpu_map[target_arch]
Line 69: 
Line 70: 
Line 71: def current():
So this is the real architecture of vdsm
Line 72:     '''
Line 73:     Get the system (host) CPU architecture.
Line 74: 
Line 75:     Returns:


Line 85:     '''
Line 86:     return _cpu_arch_to_architecture(platform.machine())
Line 87: 
Line 88: 
Line 89: def target():
And this is the effective architecture. current and target are very confusing names.
Line 90:     '''
Line 91:     Get the target VM runtime architecture. This function exists to modify the
Line 92:     architecture reported in vds capabilities and VMs. It is required because
Line 93:     some functions require to know the real architecture, while the others are


Line 98:     The runtime architecture of VDSM
Line 99:     or
Line 100:     raises UnsupportedArchitecture exception.
Line 101:     '''
Line 102:     if config.getboolean('vars', 'fake_kvm_support'):
Is this a new config? rename it to fake_kvm_enable if new.
Line 103:         return _cpu_arch_to_architecture(
Line 104:             config.get('vars', 'fake_kvm_architecture'))
Line 105:     else:
Line 106:         return current()


Line 105:     else:
Line 106:         return current()
Line 107: 
Line 108: 
Line 109: def _cpu_arch_to_architecture(target_arch):
This is confusing. In this module (cpuarch), you are using the term arch and architecture for different things.

Lets use arch only for Architecture enums.

cpu_arch in this function comes from platform.machine(), so lets call this function machine_to_architecture()
Line 110:     mapping = {
Line 111:         'x86_64': Architecture.X86_64,
Line 112:         'ppc64': Architecture.PPC64,
Line 113:         'ppc64le': Architecture.PPC64LE


Line 109: def _cpu_arch_to_architecture(target_arch):
Line 110:     mapping = {
Line 111:         'x86_64': Architecture.X86_64,
Line 112:         'ppc64': Architecture.PPC64,
Line 113:         'ppc64le': Architecture.PPC64LE
Looks like the input and the output of this function are the same. Do we really need it?
Line 114:     }
Line 115: 
Line 116:     try:
Line 117:         arch = mapping[target_arch]


-- 
To view, visit https://gerrit.ovirt.org/49972
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I46bb51769562fbb9d816d190c2aebfd0660cc3b0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list