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:36:04 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: caps/lib: move CPU architecture details to lib/cpuarch
......................................................................
Patch Set 2:
(7 comments)
Looks like a good change but:
- Moving code from caps to cpuarch, we must eliminate the Architecture class in one step - will make all the user code much nicer
- Need to improve the new names, they are not clear
- Some code looks useless
- We must remove the horrible POWER constant and replace it with explicit check for the 2 values - can be done in separate patch before this patch.
https://gerrit.ovirt.org/#/c/49972/2/lib/vdsm/cpuarch.py
File lib/vdsm/cpuarch.py:
Line 22:
Line 23: from .config import config
Line 24:
Line 25:
Line 26: class Architecture:
This class useless, we have 4 constants, putting them under a class does not help anyone. If made sense in the caps module where we had many types of constants, but not this module.
Line 27: X86_64 = 'x86_64'
Line 28: PPC64 = 'ppc64'
Line 29: PPC64LE = 'ppc64le'
Line 30: POWER = (PPC64, PPC64LE)
Line 26: class Architecture:
Line 27: X86_64 = 'x86_64'
Line 28: PPC64 = 'ppc64'
Line 29: PPC64LE = 'ppc64le'
Line 30: POWER = (PPC64, PPC64LE)
Having different types for enums is a bad idea, make the user code inconsistent.
If you need to check for power in many places, provide a predicate:
def is_power(arch):
return arch in (PPC64, PPC64le)
Line 31:
Line 32:
Line 33: class UnsupportedArchitecture(Exception):
Line 34: def __init__(self, target_arch):
https://gerrit.ovirt.org/#/c/49972/2/tests/vmTests.py
File tests/vmTests.py:
Line 212:
Line 213: for vmConf, osXML in zip(vmConfs, expectedXMLs):
Line 214: vmConf.update(self.conf)
Line 215: domxml = vmxml.Domain(vmConf, self.log,
Line 216: cpuarch.Architecture.X86_64)
Are you sure you need to break this line? other lines around look the same.
Line 217: domxml.appendOs()
Line 218: xml = find_xml_element(domxml.dom.toxml(), './os')
Line 219: self.assertXMLEqual(xml, osXML)
Line 220:
https://gerrit.ovirt.org/#/c/49972/2/vdsm/supervdsmServer
File vdsm/supervdsmServer:
Line 157: def getHardwareInfo(self, *args, **kwargs):
Line 158: if platform.machine() in ('x86_64', 'i686'):
Line 159: from vdsm.dmidecodeUtil import getHardwareInfoStructure
Line 160: return getHardwareInfoStructure()
Line 161: elif platform.machine() in cpuarch.Architecture.POWER:
Here is the proof that cpuarch._cpu_arch_to_architecture is not neeed :-)
Line 162: from vdsm.ppc64HardwareInfo import getHardwareInfoStructure
Line 163: return getHardwareInfoStructure()
Line 164: else:
Line 165: # not implemented over other architecture
https://gerrit.ovirt.org/#/c/49972/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 317
Line 318
Line 319
Line 320
Line 321
Old name is better (expect the useless get)
Line 318: self._released = False
Line 319: self._releaseLock = threading.Lock()
Line 320: self.saveState()
Line 321: self._watchdogEvent = {}
Line 322: self.arch = cpuarch.target()
I like short name, but this is not clear - target_arch()?
Line 323:
Line 324: if self.arch not in cpuarch.Architecture.POWER and \
Line 325: self.arch != cpuarch.Architecture.X86_64:
Line 326: raise RuntimeError('Unsupported architecture: %s' % self.arch)
Line 321: self._watchdogEvent = {}
Line 322: self.arch = cpuarch.target()
Line 323:
Line 324: if self.arch not in cpuarch.Architecture.POWER and \
Line 325: self.arch != cpuarch.Architecture.X86_64:
Having both string and tuple/list constants in the same enum is very bad idea.
You should do instead:
if self.arch not in (cpuarch.Architecture.PPC64,
cpuarch.Architecture.PPC64LE,
cpuarch.Architecture.X86_64):
But cpuarch.Architecture is a poor name. This Architecture class is useless.
This should have been:
if self.arch not in (cpuarch.PPC64,
cpuarch.PPC64LE,
cpuarch.X86_64):
Or even better - move the check into cpuarch, since the check is about cpuarch internals:
if cpuarch.supports_feature(self.arch):
(I don't know what you are testing here)
Line 326: raise RuntimeError('Unsupported architecture: %s' % self.arch)
Line 327:
Line 328: self._powerDownEvent = threading.Event()
Line 329: self._liveMergeCleanupThreads = {}
--
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