Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben: > 15.01.2021 14:18, Kevin Wolf wrote: > > Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Add TestEnv class, which will handle test environment in a new python > > > iotests running framework. > > > > > > Difference with current ./check interface: > > > - -v (verbose) option dropped, as it is unused > > > > > > - -xdiff option is dropped, until somebody complains that it is needed > > > - same for -n option > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > > --- > > > tests/qemu-iotests/testenv.py | 328 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 328 insertions(+) > > > create mode 100755 tests/qemu-iotests/testenv.py > > > > > > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py > > > new file mode 100755 > > > index 0000000000..ecaf76fb85 > > > --- /dev/null > > > +++ b/tests/qemu-iotests/testenv.py > > > @@ -0,0 +1,328 @@ > > > +#!/usr/bin/env python3 > > > +# > > > +# Parse command line options to manage test environment variables. > > > +# > > > +# Copyright (c) 2020 Virtuozzo International GmbH > > > +# > > > +# This program 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 warranty of > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > +# GNU General Public License for more details. > > > +# > > > +# You should have received a copy of the GNU General Public License > > > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > > > +# > > > + > > > +import os > > > +import sys > > > +import tempfile > > > +from pathlib import Path > > > +import shutil > > > +import collections > > > +import subprocess > > > +import argparse > > > +from typing import List, Dict > > > + > > > + > > > +def get_default_machine(qemu_prog: str) -> str: > > > + outp = subprocess.run([qemu_prog, '-machine', 'help'], check=True, > > > + text=True, stdout=subprocess.PIPE).stdout > > > + > > > + machines = outp.split('\n') > > > + default_machine = next(m for m in machines if m.endswith(' > > > (default)')) > > > + default_machine = default_machine.split(' ', 1)[0] > > > + > > > + alias_suf = ' (alias of {})'.format(default_machine) > > > + alias = next((m for m in machines if m.endswith(alias_suf)), None) > > > + if alias is not None: > > > + default_machine = alias.split(' ', 1)[0] > > > + > > > + return default_machine > > > + > > > + > > > +class TestEnv: > > > + """ > > > + Manage system environment for running tests > > > + > > > + The following variables are supported/provided. They are represented > > > by > > > + lower-cased TestEnv attributes. > > > + """ > > > + env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', > > > 'SAMPLE_IMG_DIR', > > > + 'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', > > > 'QEMU_IMG_PROG', > > > + 'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG', > > > + 'SOCKET_SCM_HELPER', 'QEMU_OPTIONS', > > > 'QEMU_IMG_OPTIONS', > > > + 'QEMU_IO_OPTIONS', 'QEMU_NBD_OPTIONS', 'IMGOPTS', > > > + 'IMGFMT', 'IMGPROTO', 'AIOMODE', 'CACHEMODE', > > > + 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT', > > > 'IMGFMT_GENERIC', > > > + 'IMGOPTSSYNTAX', 'IMGKEYSECRET', > > > 'QEMU_DEFAULT_MACHINE'] > > > + > > > + def get_env(self) -> Dict[str, str]: > > > + env = {} > > > + for v in self.env_variables: > > > + val = getattr(self, v.lower(), None) > > > + if val is not None: > > > + env[v] = val > > > + > > > + return env > > > + > > > + _argparser = None > > > + @classmethod > > > + def get_argparser(cls) -> argparse.ArgumentParser: > > > + if cls._argparser is not None: > > > + return cls._argparser > > > + > > > + p = argparse.ArgumentParser(description="= test environment > > > options =", > > > + add_help=False, > > > usage=argparse.SUPPRESS) > > > + > > > + p.add_argument('-d', dest='debug', action='store_true', > > > help='debug') > > > + p.add_argument('-misalign', action='store_true', > > > + help='misalign memory allocations') > > > + > > > + p.set_defaults(imgfmt='raw', imgproto='file') > > > + > > > + format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow', > > > 'qcow2', > > > + 'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', > > > 'dmg'] > > > + g = p.add_argument_group( > > > + 'image format options', > > > + 'The following options sets IMGFMT environment variable. ' > > > > s/sets/set the/ > > > > > + 'At most one chose is allowed, default is "raw"') > > > > s/chose/choice/ > > > > > + g = g.add_mutually_exclusive_group() > > > + for fmt in format_list: > > > + g.add_argument('-' + fmt, dest='imgfmt', > > > action='store_const', > > > + const=fmt) > > > + > > > + protocol_list = ['file', 'rbd', 'sheepdoc', 'nbd', 'ssh', 'nfs', > > > + 'fuse'] > > > + g = p.add_argument_group( > > > + 'image protocol options', > > > + 'The following options sets IMGPROTO environment variably. ' > > > + 'At most one chose is allowed, default is "file"') > > > > Same as above, but also s/variably/variable/. > > > > Do we consider these environment variables user interfaces? So far I > > thought of them as implementation details, but I guess if we want to > > allow users to execute test scripts manually, they are some kind of user > > interface. > > > > However, shouldn't the variables themselves then be documented > > somewhere? As it is, this feels like documenting thing X to be the same > > as thing Y, without ever saying what Y is. > > Yes, I do think that we lack some specification on the test interface, > including > environment variables.. And then, probably some unification of the interface, > to > be more clear and straightforward. But don't want to improve/refactor these > things > in these series.
Yeah, that's fair. If the new help provides at least the same information as the old one, that is good enough for me. > > > > That said... > > > > > + g = g.add_mutually_exclusive_group() > > > + for prt in protocol_list: > > > + g.add_argument('-' + prt, dest='imgproto', > > > action='store_const', > > > + const=prt) > > > > ...maybe we should just have help=f"test {fmt/prt}" here to match the > > old help text. Then this documents the option and the help above > > actually documents the environment variable. > > OK > > > > > > + > > > + g = p.add_mutually_exclusive_group() > > > + # We don't set default for cachemode, as we need to distinguish > > > dafult > > > + # from user input later. > > > + g.add_argument('-nocache', dest='cachemode', > > > action='store_const', > > > + const='none', help='set cache mode "none" > > > (O_DIRECT), ' > > > + 'sets CACHEMODE environment variable') > > > + g.add_argument('-c', dest='cachemode', > > > + help='sets CACHEMODE environment variable') > > > + > > > + p.add_argument('-i', dest='aiomode', default='threads', > > > + help='sets AIOMODE environment variable') > > > + > > > + g = p.add_argument_group('bash tests options', > > > + 'The following options are ignored by ' > > > + 'python tests. TODO: support them in ' > > > + 'iotests.py') > > > > Let's not print TODO comments to the user, but just make it a comment in > > the code. That makes it stand out better with syntax highlighting > > anyway. > > > > > + g.add_argument('-o', dest='imgopts', > > > + help='options to pass to qemu-img create/convert, > > > sets ' > > > + 'IMGOPTS environment variable') > > > + p.add_argument('-valgrind', dest='VALGRIND_QEMU', > > > action='store_const', > > > + const='y', help='use valgrind, sets VALGRIND_QEMU > > > ' > > > + 'environment variable') > > > + > > > + cls._argparser = p > > > + return p > > > + > > > + def init_handle_argv(self, argv: List[str]) -> None: > > > + > > > + # Hints for mypy, about arguments which will be set by argparse > > > > I don't understand what this comment wants to tell me. > > I mean, I could automatically set self. attributes from args, but do > it explictly to sutisfy mypy. But.. > > Actually, I'll move argv interface out of the file for v7. > > I recently learned from another project, that merging cmdline > interface (like it done her) into logic(model) class is a bad idea. (I > just had to refactor it and split ligic from the interface, to reuse > logic classes with another interface) [1] > > Also, trying to fix pylint and mypy complains, I had to inherit > classes from AbstractContextManager (for mypy).. And then, how to fix > "testenv.py:1:0: R0801: Similar lines in 2 files" ? You wandered, > should we disable it.. I think not. It's a good warning, showing bad > design. True way of fixing is creating a base class with common > functionality.. But than.. Multiple inheritance to sutisfy linters? > Haha. This brings us to [1] again. > > So, I decided to keep the classes with normal python interface > (__init__ with normal arguments), and move the whole cmdline interface > into check script itself. It looks a lot better.. Also, TestEnv is > complicated enough even without argument parsing. Ok, then I'll wait for your new version to see what it looks like. > > > > > + args, self.remaining_argv = > > > self.get_argparser().parse_known_args(argv) > > > + self.imgfmt = args.imgfmt > > > + self.imgproto = args.imgproto > > > + self.aiomode = args.aiomode > > > + self.imgopts = args.imgopts > > > + self.misalign = args.misalign > > > + self.debug = args.debug > > > + > > > + if args.cachemode is None: > > > + self.cachemode_is_default = 'true' > > > + self.cachemode = 'writeback' > > > + else: > > > + self.cachemode_is_default = 'false' > > > + self.cachemode = args.cachemode > > > + > > > + def init_directories(self): > > > + """Init directory variables: > > > + PYTHONPATH > > > + TEST_DIR > > > + SOCK_DIR > > > + SAMPLE_IMG_DIR > > > + OUTPUT_DIR > > > + """ > > > + self.pythonpath = os.getenv('PYTHONPATH') > > > + if self.pythonpath: > > > + self.pythonpath = self.source_iotests + os.pathsep + \ > > > + self.pythonpath > > > + else: > > > + self.pythonpath = self.source_iotests > > > + > > > + self.test_dir = os.getenv('TEST_DIR', > > > + os.path.join(os.getcwd(), 'scratch')) > > > + Path(self.test_dir).mkdir(parents=True, exist_ok=True) > > > + > > > + self.sock_dir = os.getenv('SOCK_DIR') > > > + self.tmp_sock_dir = False > > > + if self.sock_dir: > > > + Path(self.test_dir).mkdir(parents=True, exist_ok=True) > > > + else: > > > + self.sock_dir = tempfile.mkdtemp() > > > + self.tmp_sock_dir = True > > > + > > > + self.sample_img_dir = os.getenv('SAMPLE_IMG_DIR', > > > + os.path.join(self.source_iotests, > > > + 'sample_images')) > > > + > > > + self.output_dir = os.getcwd() # OUTPUT_DIR > > > + > > > + def init_binaries(self): > > > + """Init binary path variables: > > > + PYTHON (for bash tests) > > > + QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, > > > QSD_PROG > > > + SOCKET_SCM_HELPER > > > + """ > > > + self.python = '/usr/bin/python3 -B' > > > > This doesn't look right, we need to respect the Python binary set in > > configure (which I think we get from common.env) > > Oh, I missed the change. Then I should just drop this self.python. Do we still get the value from elsewhere or do we need to manually parse common.env? > > > > > + def root(*names): > > > + return os.path.join(self.build_root, *names) > > > + > > > + arch = os.uname().machine > > > + if 'ppc64' in arch: > > > + arch = 'ppc64' > > > + > > > + self.qemu_prog = os.getenv('QEMU_PROG', > > > root(f'qemu-system-{arch}')) > > > + self.qemu_img_prog = os.getenv('QEMU_IMG_PROG', root('qemu-img')) > > > + self.qemu_io_prog = os.getenv('QEMU_IO_PROG', root('qemu-io')) > > > + self.qemu_nbd_prog = os.getenv('QEMU_NBD_PROG', root('qemu-nbd')) > > > + self.qsd_prog = os.getenv('QSD_PROG', root('storage-daemon', > > > + > > > 'qemu-storage-daemon')) > > > + > > > + for b in [self.qemu_img_prog, self.qemu_io_prog, > > > self.qemu_nbd_prog, > > > + self.qemu_prog, self.qsd_prog]: > > > + if not os.path.exists(b): > > > + exit('Not such file: ' + b) > > > + if not os.access(b, os.X_OK): > > > + exit('Not executable: ' + b) > > > + > > > + helper_path = os.path.join(self.build_iotests, > > > 'socket_scm_helper') > > > + if os.access(helper_path, os.X_OK): > > > + self.socket_scm_helper = helper_path # SOCKET_SCM_HELPER > > > + > > > + def __init__(self, argv: List[str]) -> None: > > > + """Parse args and environment""" > > > + > > > + # Initialize generic paths: build_root, build_iotests, > > > source_iotests, > > > + # which are needed to initialize some environment variables. > > > They are > > > + # used by init_*() functions as well. > > > + > > > + > > > + if os.path.islink(sys.argv[0]): > > > + # called from the build tree > > > + self.source_iotests = > > > os.path.dirname(os.readlink(sys.argv[0])) > > > + self.build_iotests = > > > os.path.dirname(os.path.abspath(sys.argv[0])) > > > + else: > > > + # called from the source tree > > > + self.source_iotests = os.getcwd() > > > + self.build_iotests = self.source_iotests > > > + > > > + self.build_root = os.path.join(self.build_iotests, '..', '..') > > > + > > > + self.init_handle_argv(argv) > > > + self.init_directories() > > > + self.init_binaries() > > > + > > > + # QEMU_OPTIONS > > > + self.qemu_options = '-nodefaults -display none -accel qtest' > > > + machine_map = ( > > > + (('arm', 'aarch64'), 'virt'), > > > > How does this work? Won't we check for "qemu-system-('arm', 'aarch64')" > > below, which we'll never find? > > Hmm. I just took existing logic from check: > > [..] > case "$QEMU_PROG" in > *qemu-system-arm|*qemu-system-aarch64) > export QEMU_OPTIONS="$QEMU_OPTIONS -machine virt" > ;; > [..] What I mean is that the code below doesn't look like it's prepared to interpret a tuple like ('arm', 'aarch64'), it expects a single string: > > > > > + ('avr', 'mega2560'), > > > + ('rx', 'gdbsim-r5f562n8'), > > > + ('tricore', 'tricore_testboard') > > > + ) > > > + for suffix, machine in machine_map: > > > + if self.qemu_prog.endswith(f'qemu-system-{suffix}'): Here we get effectively: suffix: Tuple[str, str] = ('arm', 'aarch64') The formatted string uses str(suffix), which makes the result "qemu-system-('arm', 'aarch64')". Or am I misunderstanding something here? > > > + self.qemu_options += f' -machine {machine}' > > > + > > > + # QEMU_DEFAULT_MACHINE > > > + self.qemu_default_machine = get_default_machine(self.qemu_prog) > > > + > > > + self.qemu_img_options = os.getenv('QEMU_IMG_OPTIONS') > > > + self.qemu_nbd_options = os.getenv('QEMU_NBD_OPTIONS') > > > + > > > + is_generic = self.imgfmt not in ['bochs', 'cloop', 'dmg'] > > > + self.imgfmt_generic = 'true' if is_generic else 'false' > > > + > > > + self.qemu_io_options = f'--cache {self.cachemode} --aio > > > {self.aiomode}' > > > + if self.misalign: > > > + self.qemu_io_options += ' --misalign' > > > + > > > + self.qemu_io_options_no_fmt = self.qemu_io_options > > > + > > > + if self.imgfmt == 'luks': > > > + self.imgoptssyntax = 'true' > > > + self.imgkeysecret = '123456' > > > + if not self.imgopts: > > > + self.imgopts = 'iter-time=10' > > > + elif 'iter-time=' not in self.imgopts: > > > + self.imgopts += ',iter-time=10' > > > + else: > > > + self.imgoptssyntax = 'false' > > > + self.qemu_io_options += ' -f ' + self.imgfmt > > > + > > > + if self.imgfmt == 'vmkd': > > > + if not self.imgopts: > > > + self.imgopts = 'zeroed_grain=on' > > > + elif 'zeroed_grain=' not in self.imgopts: > > > + self.imgopts += ',zeroed_grain=on' > > > + > > > + def close(self) -> None: > > > + if self.tmp_sock_dir: > > > + shutil.rmtree(self.sock_dir) > > > + > > > + def __enter__(self) -> 'TestEnv': > > > + return self > > > + > > > + def __exit__(self, *args) -> None: > > > + self.close() > > > + > > > + def print_env(self) -> None: > > > + template = """\ > > > +QEMU -- "{QEMU_PROG}" {QEMU_OPTIONS} > > > +QEMU_IMG -- "{QEMU_IMG_PROG}" {QEMU_IMG_OPTIONS} > > > +QEMU_IO -- "{QEMU_IO_PROG}" {QEMU_IO_OPTIONS} > > > +QEMU_NBD -- "{QEMU_NBD_PROG}" {QEMU_NBD_OPTIONS} > > > +IMGFMT -- {IMGFMT}{imgopts} > > > +IMGPROTO -- {IMGPROTO} > > > +PLATFORM -- {platform} > > > +TEST_DIR -- {TEST_DIR} > > > +SOCK_DIR -- {SOCK_DIR} > > > +SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}""" > > > + > > > + args = collections.defaultdict(str, self.get_env()) > > > + > > > + if 'IMGOPTS' in args: > > > + args['imgopts'] = f" ({args['IMGOPTS']})" > > > + > > > + u = os.uname() > > > + args['platform'] = f'{u.sysname}/{u.machine} {u.nodename} > > > {u.release}' > > > + > > > + print(template.format_map(args)) > > > + > > > + > > > +if __name__ == '__main__': > > > + if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']: > > > + TestEnv.get_argparser().print_help() > > > + exit() > > > + > > > + with TestEnv(sys.argv) as te: > > > + te.print_env() > > > + print('\nUnhandled options: ', te.remaining_argv) > > Thanks for reviewing! I hope to post v7 today, with cmdline interface > in 'check' script. Sounds good. Then I'll continue the review when v7 is on the list. Kevin