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