|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd' |
Date: | Mon, 19 Jul 2021 15:58:49 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 |
16.07.2021 13:34, Vladimir Sementsov-Ogievskiy wrote:
16.07.2021 13:07, Max Reitz wrote:On 05.07.21 11:15, Vladimir Sementsov-Ogievskiy wrote:Adding support of IMGOPTS (like in bash tests) allows user to pass a lot of different options. Still, some may require additional logic. Now we want compression_type option, so add some smart logic around it: ignore compression_type=zstd in IMGOPTS, if test want qcow2 in compatibility mode. As well, ignore compression_type for non-qcow2 formats. Note that we may instead add support only to qemu_img_create(), but that works bad: 1. We'll have to update a lot of tests to use qemu_img_create instead of qemu_img('create'). (still, we may want do it anyway, but no reason to create a dependancy between task of supporting IMGOPTS and updating a lot of tests) 2. Some tests use qemu_img_pipe('create', ..) - even more work on updatingI feel compelled to again say that we had a series that did exactly that. But of course, now that so much time has passed, overhauling it would require quite a bit of work.3. Even if we update all tests to go through qemu_img_create, we'll need a way to avoid creating new tests using qemu_img*('create') - add assertions.. That doesn't seem good.That almost sounds like you remember my series, because: https://lists.nongnu.org/archive/html/qemu-block/2019-10/msg00135.html ;):) No, I don't remember it.So, let's add support of IMGOPTS to most generic qemu_img_pipe_and_status(). Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com> --- tests/qemu-iotests/iotests.py | 48 ++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 0d99dd841f..80f0cb4f42 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -16,6 +16,7 @@ # along with this program. If not, see<http://www.gnu.org/licenses/>. # +import argparse import atexit import bz2 from collections import OrderedDict @@ -41,6 +42,19 @@ from qemu.machine import qtest from qemu.qmp import QMPMessage + +def optstr2dict(opts: str) -> Dict[str, str]: + if not opts: + return {} + + return {arr[0]: arr[1] for arr in + (opt.split('=', 1) for opt in opts.strip().split(','))} + + +def dict2optstr(opts: Dict[str, str]) -> str: + return ','.join(f'{k}={v}' for k, v in opts.items()) + + # Use this logger for logging messages directly from the iotests module logger = logging.getLogger('qemu.iotests') logger.addHandler(logging.NullHandler()) @@ -57,6 +71,8 @@ if os.environ.get('QEMU_IMG_OPTIONS'): qemu_img_args += os.environ['QEMU_IMG_OPTIONS'].strip().split(' ') +imgopts = optstr2dict(os.environ.get('IMGOPTS', '')) + qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')] if os.environ.get('QEMU_IO_OPTIONS'): qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ') @@ -121,11 +137,41 @@ def qemu_tool_pipe_and_status(tool: str, args: Sequence[str], {-subp.returncode}: {cmd}\n') return (output, subp.returncode) +def qemu_img_create_prepare_args(args: List[str]) -> List[str]: + if not args or args[0] != 'create': + return list(args) + args = args[1:] + + p = argparse.ArgumentParser(allow_abbrev=False) + # -o option may be specified several times + p.add_argument('-o', action='append', default=[]) + p.add_argument('-f') + parsed, remaining = p.parse_known_args(args) + + opts = optstr2dict(','.join(parsed.o)) + + compat = 'compat' in opts and opts['compat'][0] == '0'I suppose `opts['compat'][0] == '0'` is supposed to check for compat=0.10? If so, then why not just check `opts['compat'] == '0.10'` to be clearer? I don’t think we allow any other compat=0* values, and I have no reason to believe we ever will. Also, I think compat=v2 is valid, too. (And I think calling this variable “v2” would also make more sense than “compat”.)Good for me, will do.+ for k, v in imgopts.items(): + if k in opts: + continue + if k == 'compression_type' and (compat or parsed.f != 'qcow2'): + continue + opts[k] = vCould also be done with something like imgopts = os.environ.get('IMGOPTS')imgopts is a string after it. So you don't need to join it?opts = optstr2dict(','.join(([imgopts] if imgopts else []) + parsed.o))Build a string to be than parsed looks strange IMHO..
Oh, but that's exactly what I should do anyway to cover several -o options. Now I see that what you write is correct.
if parsed.f != 'qcow2' or (opts.get('compat') in ['v2', '0.10']): opts.pop('compression_type', None) (Never tested, of course) Because optstr2dict() prioritizes later options over earlier ones. (Which is good, because that’s also qemu-img’s behavior.)Ok, I'll think about this all when prepare v2, and we'll see how it goes+ + result = ['create'] + if parsed.f is not None: + result += ['-f', parsed.f]Can this even be None? I hope none of our tests do this.I'm afraid they do, as I first wanted to make a default to be imgfmt, but faced tests that rely on default being raw.. May be I'm wrong. Will check it.+ if opts: + result += ['-o', dict2optstr(opts)] + result += remaining + + return result + def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]: """ Run qemu-img and return both its output and its exit code """ - full_args = qemu_img_args + list(args) + full_args = qemu_img_args + qemu_img_create_prepare_args(list(args)) return qemu_tool_pipe_and_status('qemu-img', full_args) def qemu_img(*args: str) -> int:There’s also qemu_img_verbose() which I think should be amended the same way (even if it’s currently never used for 'create').Right, thanks. I think better is rewrite qemu_img_verbose to to call qemu_img_pipe_and_status. Another thing to do is moving handling luks from qemu_img_create to qemu_img_create_prepare_args, so all these things be in one place.
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |