Change in vdsm[master]: vdsm: introduce cpuinfo module

danken at redhat.com danken at redhat.com
Tue Nov 17 12:02:08 UTC 2015


Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: introduce cpuinfo module
......................................................................


Patch Set 13: Code-Review-1

(6 comments)

https://gerrit.ovirt.org/#/c/46912/13/lib/vdsm/cpuarch.py
File lib/vdsm/cpuarch.py:

Line 33: _CPU_ARCH_TO_ARCHITECTURE = {
Line 34:     'x86_64': Architecture.X86_64,
Line 35:     'ppc64': Architecture.PPC64,
Line 36:     'ppc64le': Architecture.PPC64LE
Line 37: }
this confuses me, as the dictionary maps x->x for all keys.

This seems to be used only as a set of known archs. Could you limit it to its single use case inside current()?
Line 38: 
Line 39: # CPU_MAP is the actual libvirt cpu_map.xml; the values x86 and ppc64 are used
Line 40: # instead of x86_64 or ppc64le.
Line 41: _ARCHITECTURE_TO_CPU_MAP = {


Line 79: 
Line 80: 
Line 81: def current(target_arch=None):
Line 82:     '''
Line 83:     Get the CPU architecture.
sorry, I do not understand the subtle differences between current() and target(). Can you rewrite the docstring?
Line 84: 
Line 85:     Arguments:
Line 86: 
Line 87:     target_arch Optional. Accepts a string of one of the supported


https://gerrit.ovirt.org/#/c/46912/13/lib/vdsm/cpuinfo.py
File lib/vdsm/cpuinfo.py:

Line 43: _CPUS_LOCK
I don't think we need the lock here. Instead, have this function RETURN _CPUS and trust memoized() to do its thing properly.


Line 67:     raises UnsupportedArchitecture exception or KeyError if cpuinfo format
Line 68:     is invalid.
Line 69: 
Line 70:     '''
Line 71:     if not _CPUS:
it should be simpler to do

 _CPUS = parse()

here.
Line 72:         parse()
Line 73: 
Line 74:     if cpuarch.current() == cpuarch.Architecture.X86_64:
Line 75:         return _CPUS[0]['flags'].split()


https://gerrit.ovirt.org/#/c/46912/13/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 783: 
this has been dropped by this patch. it might be harmless, but please take note.


https://gerrit.ovirt.org/#/c/46912/13/tests/nettestlib.py
File tests/nettestlib.py:

Line 155:     _IFF_TAP = 0x0002
Line 156:     _IFF_NO_PI = 0x1000
Line 157:     if cpuarch.current() == cpuarch.Architecture.X86_64:
Line 158:         _TUNSETIFF = 0x400454ca
Line 159:     elif cpuarch.current() == 'ppc64':
why not use the cpuarch.Architecture enum here?
Line 160:         _TUNSETIFF = 0x800454ca
Line 161:     else:
Line 162:         raise SkipTest("Unsupported Architecture %s" % cpuarch.current())
Line 163: 


-- 
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: 13
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: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list