Francesco Romani has posted comments on this change. Change subject: vdsm: isolate cpu info and architecture in a new module ......................................................................
Patch Set 1: (6 comments) I like this direction 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? 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? 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. For the future, I'd rather have cpuinfo.arch() 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) 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 here. Line 103: if arch == 'x86_64': Line 104: _TUNSETIFF = 0x400454ca Line 105: elif arch == 'ppc64': Line 106: _TUNSETIFF = 0x800454ca 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 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' -- 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: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
