Martin Polednik has posted comments on this change. Change subject: vdsm: isolate cpu info and architecture in a new module ......................................................................
Patch Set 1: (8 comments) https://gerrit.ovirt.org/#/c/46912/1/lib/vdsm/cpuinfo.py File lib/vdsm/cpuinfo.py: Line 20: Line 21: import platform Line 22: Line 23: Line 24: class UnsupportedArchitecture(RuntimeError): > why RuntimeError and not, let's say, Exception? Since RuntimeError was raised previously, and raising more general exception may lead to some breakages. It's not really caught anywhere in our code afaik, so no strong opinion. Line 25: pass Line 26: Line 27: Line 28: class Architecture: Line 95: Line 96: arch = _cpu_arch_to_architecture[platform.machine()] Line 97: flags = _flags() Line 98: mhz = _mhz() Line 99: model = _model() > not sure I like this. What's the drawback having them as plain functions? Small performance and aesthetic differences; I was actually considering memoized functions but in case of required refresh (that's why 'parse' is exposed) we don't really have the tools for that. From code aesthetics, it seems cleaner to call cpuinfo.mhz then cpuinfo.mhz(). Not really a strong opinion and something I'm wiling to change. https://gerrit.ovirt.org/#/c/46912/1/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 808: if os.path.exists(constants.P_VDSM_NODE_ID): Line 809: with open(constants.P_VDSM_NODE_ID) as f: Line 810: __hostUUID = f.readline().replace("\n", "") Line 811: else: Line 812: arch = cpuinfo.arch > as it is right now, we don't need this temporary. Done Line 813: if arch in (cpuinfo.Architecture.X86_64): Line 814: ret, out, err = execCmd([constants.EXT_DMIDECODE, Line 815: "-s", Line 816: "system-uuid"], https://gerrit.ovirt.org/#/c/46912/1/tests/functional/virtTests.py File tests/functional/virtTests.py: Line 230: @requireKVM Line 231: def testSimpleVm(self): Line 232: customization = {'vmId': '77777777-ffff-3333-bbbb-222222222222', Line 233: 'vmName': 'testSimpleVm', Line 234: 'display': _GRAPHICS_FOR_ARCH[cpuinfo.arch]} > could be a class constant (no big deal however) can consider that in separate patch Line 235: Line 236: with RunningVm(self.vdsm, customization) as vm: Line 237: self._waitForStartup(vm, VM_MINIMAL_UPTIME) Line 238: self._verifyDevices(vm) https://gerrit.ovirt.org/#/c/46912/1/tests/nettestlib.py File tests/nettestlib.py: Line 98: class Tap(Interface): Line 99: Line 100: _IFF_TAP = 0x0002 Line 101: _IFF_NO_PI = 0x1000 Line 102: arch = cpuinfo.arch > same comment: if it is exposed as variable/constant, no need for temporary Done Line 103: if arch == 'x86_64': Line 104: _TUNSETIFF = 0x400454ca Line 105: elif arch == 'ppc64': Line 106: _TUNSETIFF = 0x800454ca Line 101: _IFF_NO_PI = 0x1000 Line 102: arch = cpuinfo.arch Line 103: if arch == 'x86_64': Line 104: _TUNSETIFF = 0x400454ca Line 105: elif arch == 'ppc64': possibly missing ppc64le Line 106: _TUNSETIFF = 0x800454ca Line 107: else: Line 108: raise SkipTest("Unsupported Architecture %s" % arch) Line 109: https://gerrit.ovirt.org/#/c/46912/1/vdsm/caps.py File vdsm/caps.py: Line 382: with open(capfile) as xml: Line 383: cpu_map = ET.fromstring(xml.read()) Line 384: Line 385: if arch is None: Line 386: arch = cpuinfo.arch > same, no need for temporary given the current code Done Line 387: Line 388: # In libvirt CPU map XML, both x86_64 and x86 are Line 389: # the same architecture, so in order to find all Line 390: # the CPU models for this architecture, 'x86' Line 392: if arch == cpuinfo.Architecture.X86_64: Line 393: arch = 'x86' Line 394: Line 395: # Same goes for ppc64le Line 396: if arch == cpuinfo.Architecture.PPC64LE: For robustness sake, should be in cpuinfo.Architecture.POWER Line 397: arch = 'ppc64' Line 398: Line 399: architectureElement = None Line 400: -- To view, visit https://gerrit.ovirt.org/46912 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa702b05f3825ebdcfed16d86d39a8c38fcf224c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
