On 03/26/2012 02:23 PM, Peter Krempa wrote: > On 03/24/2012 06:42 PM, Martin Kletzander wrote: >> In order to support libvirt-test-API on more distributions, this >> commit adds support for Gentoo. >> The file is copy-paste from dist/redhat/env_update.py just modified to >> make the get_* functions work on Gentoo, some removed. > > Probably no one else that uses the test api uses Gentoo, so I'll do a > review of this patch. > >> --- >> dist/gentoo/env_inspect.py | 98 >> ++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 98 insertions(+), 0 deletions(-) >> create mode 100644 dist/gentoo/__init__.py >> create mode 100644 dist/gentoo/env_inspect.py >> >> diff --git a/dist/gentoo/__init__.py b/dist/gentoo/__init__.py >> new file mode 100644 >> index 0000000..e69de29 >> diff --git a/dist/gentoo/env_inspect.py b/dist/gentoo/env_inspect.py >> new file mode 100644 >> index 0000000..e8fccc0 >> --- /dev/null >> +++ b/dist/gentoo/env_inspect.py >> @@ -0,0 +1,98 @@ >> +#!/usr/bin/env python >> +# >> +# libvirt-test-API is copyright 2010, 2012 Red Hat, Inc. > > s/2010, // > We should have 2010, 2012 or 2010-2012 in the file, not just 2010, I guess. >> +# libvirt-test-API is free software: you can redistribute it and/or >> modify it >> +# under the terms of the GNU General Public License as published by >> +# the Free Software Foundation, either version 2 of the License, or >> +# (at your option) any later version. This program is distributed in >> +# the hope that it will be useful, but WITHOUT ANY WARRANTY; without >> +# even the implied warranties of TITLE, NON-INFRINGEMENT, >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. >> +# >> +# The GPL text is available in the file COPYING that accompanies this >> +# distribution and at<http://www.gnu.org/licenses>. >> +# >> +# Filename: envinspect.py > > s/envinspect/env_inspect/ > >> +# Summary: To generate a callable class for clearing testing environment >> +# Description: The module match the reference of clearing function >> +# from each testcase to the corresponding testcase's >> +# argument in the order of testcase running > > A little re-wording would help to understand what's going on here. > This and others are just a copy-paste from redhat/env_inspect.py and I didn't write it but when writing v2, I'll try to rephrase this. >> + >> +import os >> +import portage >> + >> +vt_dbapi = portage.db[portage.root]['vartree'].dbapi >> + >> +def get_libvirt_ver(): >> + pkg = vt_dbapi.match('app-emulation/libvirt') >> + if pkg: >> + return 0, pkg[-1] >> + else: >> + return 100, "No libvirt installed" >> + >> +def get_libvirt_pyth_ver(): >> + pkgs = vt_dbapi.match('app-emulation/libvirt') >> + for pkg in pkgs: >> + if 'python' in vt_dbapi.aux_get(pkg, ['USE'])[0].split(): >> + return 0, '%s[python]' % pkg >> + >> + return 100, "USE flag 'python' not enabled for libvirt" >> + >> +def get_qemu_kvm_ver(): >> + pkg = vt_dbapi.match('qemu-kvm') or vt_dbapi.match('qemu') >> + if pkg: >> + return 0, pkg[-1] >> + else: >> + return 100, "No qemu installed" > > This function returns codes 0 or 100 ... > >> + >> +def get_kernel_ver(): >> + # on Gentoo, there is no need to check for kernel >> + return 0, os.uname()[2] >> + >> + >> +class EnvInspect(object): >> + """to check and collect the testing enviroment infomation >> + before performing testing >> + """ >> + >> + def __init__(self, logger): >> + self.logger = logger >> + >> + def env_checking(self): >> + flag = 0 >> + result = "" >> + if get_libvirt_ver()[0] == 100: >> + result = NOTOK >> + flag = 1 >> + else: >> + result = OK >> + self.logger.info(" %-36s%-6s" % (get_libvirt_ver()[1], >> result)) >> + >> + if get_libvirt_pyth_ver()[0] == 100: >> + result = NOTOK >> + flag = 1 >> + else: >> + result = OK >> + self.logger.info(" %-36s%-6s" % >> (get_libvirt_pyth_ver()[1], result)) >> + >> + if get_qemu_kvm_ver()[0] == 150 and flag == 0: >> + flag = 0 >> + elif get_qemu_kvm_ver()[0] == 150 and flag == 1: >> + flag = 1 > > ... so these tests here are not needed. (And are quite strange too ... > but that's probably a cut&paste leftover) > >> + else: >> + pass >> + self.logger.info(" %-36s%-6s" % (get_qemu_kvm_ver()[1], OK)) > > And in any case, the test succeeds, so it's probably ment to be only a > version check, that is not mandatory. IMO we should make this check > mandatory for now, as most of the tests use the local hypervisor for > testing machines and only a few tests actualy deal with remote code. > >> + >> + if get_kernel_ver()[0] == 100: >> + result = NOTOK >> + flag = 1 >> + else: > > Well, as was said in the comment, on Gentoo the kernel test always > succeeds, so the return value check is not necessary and can be changed > to something like: > > self.logger.info(" %-36s%-6s" % (get_kernel_ver()[1], OK)) > > and leave out the condition. I wanted to completely rewrite this (I don't understand why we use return codes like 0 and 1 instead of True and False, codes 0 and 100, then check it for 150, call the functions twice, etc.), however the main goal was to make it work. Nevertheless I'll rewrite this. >> + result = OK >> + self.logger.info(" %-36s%-6s" % (get_kernel_ver()[1], >> result)) >> + >> + return flag >> + >> + >> +OK = "ok" >> +NOTOK = "not ok" > > In any case, it works correctly on Gentoo and the tests are faster than > querying the package states with equery. > > Probably worth a v2 where you change the test conditions. > > Peter > Will do. Thanks. Martin -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list