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

nsoffer at redhat.com nsoffer at redhat.com
Wed Jan 20 07:53:19 UTC 2016


Nir Soffer has posted comments on this change.

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


Patch Set 23:

(2 comments)

Not sure why introducing the cpuinfo module require adding new functions to the cpuarch module.

Can't we introduce this module and the tests in a separate patch?

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

Line 106:     or
Line 107:     raises UnsupportedArchitecture exception.
Line 108:     '''
Line 109:     if not target_arch:
Line 110:         target_arch = real()
Do not allow calling with None or empty target_arch like this - this is a require argument. If you want to make it optional, use kwarg:

    def arch_to_libvist_cpu_map(target_arch=None):
        ...

But better not allow empty or None argument, and let the caller choose what it like to convert, real() or effective() or something else.
Line 111: 
Line 112:     # CPU_MAP is the actual libvirt cpu_map.xml; the values x86 and ppc64 are
Line 113:     # used instead of x86_64 or ppc64le.
Line 114:     _architecture_to_cpu_map = {


Line 111: 
Line 112:     # CPU_MAP is the actual libvirt cpu_map.xml; the values x86 and ppc64 are
Line 113:     # used instead of x86_64 or ppc64le.
Line 114:     _architecture_to_cpu_map = {
Line 115:         X86_64: 'x86', PPC64: 'ppc64', PPC64LE: 'ppc64'}
This creates a new dictionary on each call to this function. This should be quick, but is wasteful.

For 3 values, using simple if - elif - else is simpler and better. Using a dict makes sense when we have more items and we want to make it easy to add new items without modifying the code. 

If you think the we need the dict, create it as private module variable.
Line 116: 
Line 117:     try:
Line 118:         return _architecture_to_cpu_map[target_arch]
Line 119:     except KeyError:


-- 
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: 23
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