Yaniv Bronhaim has posted comments on this change.
Change subject: Capabilities: List capabilities of the IBM POWER familiy
......................................................................
Patch Set 3:
(10 comments)
....................................................
Commit Message
Line 3: AuthorDate: 2013-07-30 08:48:12 -0300
Line 4: Commit: Vitor de Lima <vitor.l...@eldorado.org.br>
Line 5: CommitDate: 2013-08-30 09:05:23 -0300
Line 6:
Line 7: Capabilities: List capabilities of the IBM POWER familiy
The "Capabilities:" prefix is redundant
s/familiy/family
it can be just "adding ppc64 handling to getVdsCaps"
Line 8:
Line 9: This introduces a parser capable of interpreting the /proc/cpuinfo
Line 10: present in IBM POWER hosts. It also retrieves the possible emulated
Line 11: machines for QEMU guests. The POWER 7 processors are not compatible
Line 17:
Line 18: A upcoming 'fake KVM on POWER' mode is partially included, adding
Line 19: a configuration parameter indicating to VDSM if it must report fake
Line 20: capabilities, so a x86_64 host could be used to validate POWER specific
Line 21: code.
IMHO, you need to split your change to 2 patches, one adding the parsing of
/proc/cpuinfo if using ppc architecture, and one for adding fake kvm that
emulates ppc
Line 22:
Line 23: Change-Id: I42c4d00ace06805edbe765d975b40c9311a1fa9b
....................................................
File lib/vdsm/config.py.in
Line 141: ('qemu_drive_cache', 'none', None),
Line 142:
Line 143: ('fake_kvm_support', 'false', None),
Line 144:
Line 145: ('fake_kvm_architecture', 'x86_64', None),
why None? please add short description
Line 146:
Line 147: ('xmlrpc_enable', 'true', 'Enable the xmlrpc server'),
Line 148:
Line 149: ('jsonrpc_enable', 'true', 'Enable the JSON RPC server'),
....................................................
File tests/capsTests.py
Line 30: def testCpuInfo(self):
Line 31: testPath = os.path.realpath(__file__)
Line 32: dirName = os.path.split(testPath)[0]
Line 33: path = os.path.join(dirName, "cpu_info.out")
Line 34: c = caps.CpuInfo(path, 'x86_64')
use platform.machine()
Line 35: self.assertEqual(set(c.flags()), set("""fpu vme de pse tsc msr
pae
Line 36: mce cx8 apic mtrr pge
mca
Line 37: cmov pat pse36 clflush
dts
Line 38: acpi mmx fxsr sse sse2
ss ht
....................................................
File vdsm/caps.py
Line 65: DEBIAN = 'Debian'
Line 66:
Line 67:
Line 68: class CpuInfo(object):
Line 69: def __init__(self, cpuinfo='/proc/cpuinfo', hostArch='x86_64'):
you can't set default to x86_64.. how is it default? you always can use
platform.machine() , why do you need default ?
Line 70: """Parse /proc/cpuinfo"""
Line 71: self._info = {}
Line 72: p = {}
Line 73: self.arch = hostArch
Line 81: self._info[value] = p
Line 82: else:
Line 83: p[key] = value
Line 84:
Line 85: def flags(self):
switch case with default section for handling errors more fits here
Line 86: if self.arch == 'x86_64':
Line 87: return self._info.itervalues().next()['flags'].split()
Line 88: elif self.arch == 'ppc64':
Line 89: return ['powernv']
Line 82: else:
Line 83: p[key] = value
Line 84:
Line 85: def flags(self):
Line 86: if self.arch == 'x86_64':
you repeat using 'x86_64' and 'ppc64' all over, please use constant for both
strings.
Line 87: return self._info.itervalues().next()['flags'].split()
Line 88: elif self.arch == 'ppc64':
Line 89: return ['powernv']
Line 90:
Line 161: cpu_map = minidom.parseString(
Line 162: file('/usr/share/libvirt/cpu_map.xml').read())
Line 163:
Line 164: if arch == 'x86_64':
Line 165: arch = 'x86'
explain this part in a comment please, I don't understand it
Line 166:
Line 167: architectureElements = cpu_map.getElementsByTagName('arch')
Line 168:
Line 169: allModels = dict()
Line 194:
Line 195: @utils.memoized
Line 196: def _getCompatibleCpuModels(arch):
Line 197:
Line 198: if arch == 'ppc64':
I prefer to move all this code to external package that handles all ppc64
information.
Line 199: # The entire POWER7 family is incompatible with each other, so
Line 200: # it is enough to list only the host processor and its version
Line 201: # This is also a hack around libvirt BZ#988077
Line 202:
Line 295:
Line 296:
Line 297: def get():
Line 298: hostArch = platform.machine()
Line 299:
import platform only if needed, under the else scope
Line 300: if config.getboolean('vars', 'fake_kvm_support'):
Line 301: targetArch = config.get('vars', 'fake_kvm_architecture')
Line 302: else:
Line 303: targetArch = hostArch
--
To view, visit http://gerrit.ovirt.org/17437
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c4d00ace06805edbe765d975b40c9311a1fa9b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vitor de Lima <vitor.l...@eldorado.org.br>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
