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 <mpoled...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches