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

nsoffer at redhat.com nsoffer at redhat.com
Sun Jan 17 01:23:44 UTC 2016


Nir Soffer has posted comments on this change.

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


Patch Set 22:

(11 comments)

Much nicer then the previous code, but can be even simpler.

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

Line 1: #
Line 2: # Copyright 2015 Red Hat, Inc.
Its 2016 now.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or


Line 24: 
Line 25: from . import cpuarch
Line 26: 
Line 27: 
Line 28: _SOURCE = '/proc/cpuinfo'
This looks more like _PATH
Line 29: _CPU_TEMPLATE = namedtuple('_CPU_TEMPLATE', 'flags, frequency, model')
Line 30: 
Line 31: 
Line 32: @utils.memoized


Line 25: from . import cpuarch
Line 26: 
Line 27: 
Line 28: _SOURCE = '/proc/cpuinfo'
Line 29: _CPU_TEMPLATE = namedtuple('_CPU_TEMPLATE', 'flags, frequency, model')
Why not CpuInfo? why use constant name for a class?
Line 30: 
Line 31: 
Line 32: @utils.memoized
Line 33: def parse(source=_SOURCE):


Line 29: _CPU_TEMPLATE = namedtuple('_CPU_TEMPLATE', 'flags, frequency, model')
Line 30: 
Line 31: 
Line 32: @utils.memoized
Line 33: def parse(source=_SOURCE):
source parameter is not needed. For testing, we can monkeypatch cpuinfo._SOURCE.

Also, why is this public? this should be a private function.

Finally, if you rename it to _cpuinfo(), we can write the accessors like this:

    def flags():
        return _cpuinfo().flags

Which is little nicer.
Line 34:     '''
Line 35:     Parse cpuinfo-like file, keeping the values in module's runtime variables.
Line 36: 
Line 37:     Arguments:


Line 38: 
Line 39:     source      Optional. Accepts a string indicating path to the cpuinfo-like
Line 40:                 file. If not supplied, default path (/proc/cpuinfo) is used.
Line 41:     '''
Line 42:     parsed_fields = [['powernv'], None, None]
Use a dict:

   fields = {}
   
On ppc, add the flags before parsing:

    if cpuarch.is_ppc(arch):
        fields["flags"] = ["powernv"]
Line 43:     counter_stop = len(parsed_fields)
Line 44:     counter = 0
Line 45: 
Line 46:     if cpuarch.is_ppc(cpuarch.real()):


Line 39:     source      Optional. Accepts a string indicating path to the cpuinfo-like
Line 40:                 file. If not supplied, default path (/proc/cpuinfo) is used.
Line 41:     '''
Line 42:     parsed_fields = [['powernv'], None, None]
Line 43:     counter_stop = len(parsed_fields)
Unneeded
Line 44:     counter = 0
Line 45: 
Line 46:     if cpuarch.is_ppc(cpuarch.real()):
Line 47:         counter += 1


Line 40:                 file. If not supplied, default path (/proc/cpuinfo) is used.
Line 41:     '''
Line 42:     parsed_fields = [['powernv'], None, None]
Line 43:     counter_stop = len(parsed_fields)
Line 44:     counter = 0
Uneeded
Line 45: 
Line 46:     if cpuarch.is_ppc(cpuarch.real()):
Line 47:         counter += 1
Line 48: 


Line 50:         for line in info:
Line 51:             if not line.strip():
Line 52:                 continue
Line 53: 
Line 54:             key, value = map(str.strip, line.split(':', 1))
Use:

    [part.strip() for part in line.split(":", 1)]
Line 55: 
Line 56:             if key == 'flags':
Line 57:                 parsed_fields[0] = value.split()
Line 58:                 counter += 1


Line 69:                 parsed_fields[2] = value
Line 70:                 counter += 1
Line 71: 
Line 72:             if counter == counter_stop:
Line 73:                 break
Use instead:

    if len(fields) == 3:
        break
Line 74: 
Line 75:         return _CPU_TEMPLATE(*parsed_fields)
Line 76: 
Line 77: 


Line 71: 
Line 72:             if counter == counter_stop:
Line 73:                 break
Line 74: 
Line 75:         return _CPU_TEMPLATE(*parsed_fields)
Use:

    return _CPU_TEMPLATE(**fields)
Line 76: 
Line 77: 
Line 78: def flags():
Line 79:     '''


Line 89:     '''
Line 90:     return parse(_SOURCE).flags
Line 91: 
Line 92: 
Line 93: def mhz():
Rename to frequency()
Line 94:     '''
Line 95:     Get the CPU frequency.
Line 96: 
Line 97:     Returns:


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