Nir Soffer has posted comments on this change. Change subject: caps/lib: move CPU architecture details to lib/cpuarch ......................................................................
Patch Set 3: (14 comments) I like this. Need to fix some leftovers from previous version, please see the comments. https://gerrit.ovirt.org/#/c/49972/3/lib/vdsm/cpuarch.py File lib/vdsm/cpuarch.py: Line 26: X86_64 = 'x86_64' Line 27: PPC64 = 'ppc64' Line 28: PPC64LE = 'ppc64le' Line 29: Line 30: SUPPORTED_ARCHITECTURES = ('x86_64', 'ppc64', 'ppc64le') Nice, but we should use the constants, not the actual values. Line 31: Line 32: Line 33: class UnsupportedArchitecture(Exception): Line 34: def __init__(self, target_arch): Line 50: raises UnsupportedArchitecture exception. Line 51: Line 52: Examples: Line 53: Line 54: current() ~> Architecture.X86_64 Architecture does not exists now. Line 55: ''' Line 56: return _supported_arch(platform.machine()) Line 57: Line 58: Line 52: Examples: Line 53: Line 54: current() ~> Architecture.X86_64 Line 55: ''' Line 56: return _supported_arch(platform.machine()) So we want this code to fail when running on unsupported plarform, e.g. arm? Line 57: Line 58: Line 59: def effective(): Line 60: ''' Line 83: def is_x86(arch): Line 84: return arch == X86_64 Line 85: Line 86: Line 87: def _supported_arch(target_arch): Calling this _supported() would be little nicer and more consistent with the other names in this module (effective(), real(), ...). Line 88: if target_arch in SUPPORTED_ARCHITECTURES: Line 89: return target_arch Line 90: else: Line 87: def _supported_arch(target_arch): Line 88: if target_arch in SUPPORTED_ARCHITECTURES: Line 89: return target_arch Line 90: else: Line 91: raise UnsupportedArchitecture(target_arch) This function is not about 2 options, supported and not supported, but about validating the parameter and raising UnsupportedArchitecture for unsupported values. So the usual idiom of failing fast apply: if target_arch not in SUPPORTED_ARCHITECTURES: raise ... return target_arch https://gerrit.ovirt.org/#/c/49972/3/tests/vmTests.py File tests/vmTests.py: Line 1313: def _getAllDomainIds(self, arch): Line 1314: return [conf['vmId'] for conf, _ in self._CONFS[arch]] Line 1315: Line 1316: @permutations([[cpuarch.X86_64], Line 1317: [cpuarch.PPC64]]) The line brake is not needed (leftover from previous version?) Line 1318: def testGetVDSMDomains(self, arch): Line 1319: with MonkeyPatchScope([(vm, '_listDomains', Line 1320: lambda: self._buildAllDomains(arch)), Line 1321: (cpuarch, 'effective', lambda: arch)]): https://gerrit.ovirt.org/#/c/49972/3/tests/vmXmlTests.py File tests/vmXmlTests.py: Line 50: @expandPermutations Line 51: class TestVmXmlFunctions(VmXmlTestCase): Line 52: Line 53: @permutations([[cpuarch.X86_64], Line 54: [cpuarch.PPC64]]) We can use single line now. Line 55: def test_has_channel(self, arch): Line 56: for _, dom_xml in self._build_domain_xml(arch): Line 57: self.assertEqual(True, vmxml.has_channel( Line 58: dom_xml, vm._VMCHANNEL_DEVICE_NAME)) https://gerrit.ovirt.org/#/c/49972/3/vdsm/caps.py File vdsm/caps.py: Line 106: else: Line 107: p[key] = value Line 108: Line 109: def flags(self): Line 110: if self._arch == cpuarch.X86_64: there is not need to change the check, use your new shiny cpuarch.is_x86() Same for all other checks in this module. Line 111: return self._info.itervalues().next()['flags'].split() Line 112: elif self._arch in cpuarch.POWER: Line 113: return ['powernv'] Line 114: else: Line 108: Line 109: def flags(self): Line 110: if self._arch == cpuarch.X86_64: Line 111: return self._info.itervalues().next()['flags'].split() Line 112: elif self._arch in cpuarch.POWER: There is no such constant now - use: elif cpuarch.is_ppc(self._arch): Same for all other checks in this module. Line 113: return ['powernv'] Line 114: else: Line 115: raise RuntimeError('Unsupported architecture') Line 116: Line 398: # In libvirt CPU map XML, both x86_64 and x86 are Line 399: # the same architecture, so in order to find all Line 400: # the CPU models for this architecture, 'x86' Line 401: # must be used Line 402: if arch == cpuarch.X86_64: Use here is_x86() Line 403: arch = 'x86' Line 404: Line 405: # Same goes for ppc64le Line 406: if arch == cpuarch.PPC64LE: Line 402: if arch == cpuarch.X86_64: Line 403: arch = 'x86' Line 404: Line 405: # Same goes for ppc64le Line 406: if arch == cpuarch.PPC64LE: This check is correct. Line 407: arch = 'ppc64' Line 408: Line 409: architectureElement = None Line 410: https://gerrit.ovirt.org/#/c/49972/3/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.POWER: No POWER now, use is_ppc() 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/3/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 318: self._released = False Line 319: self._releaseLock = threading.Lock() Line 320: self.saveState() Line 321: self._watchdogEvent = {} Line 322: self.arch = cpuarch.effective() This will raise now UnsupportedArchitecture... Line 323: Line 324: if not cpuarch.is_ppc(self.arch) and not cpuarch.is_x86(self.arch): Line 325: raise RuntimeError('Unsupported architecture: %s' % self.arch) Line 326: Line 320: self.saveState() Line 321: self._watchdogEvent = {} Line 322: self.arch = cpuarch.effective() Line 323: Line 324: if not cpuarch.is_ppc(self.arch) and not cpuarch.is_x86(self.arch): So this check can be dropped. Rasing cpuarch.UnsupportedArchitecture is about 1000 times better than raising RuntimeError. Line 325: raise RuntimeError('Unsupported architecture: %s' % self.arch) Line 326: Line 327: self._powerDownEvent = threading.Event() Line 328: 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
