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