| Message ID | 20210705091549.178335-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: support zstd | expand |
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 > updating I 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 ;) > 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”.) > + for k, v in imgopts.items(): > + if k in opts: > + continue > + if k == 'compression_type' and (compat or parsed.f != 'qcow2'): > + continue > + opts[k] = v Could also be done with something like imgopts = os.environ.get('IMGOPTS') opts = optstr2dict(','.join(([imgopts] if imgopts else []) + parsed.o)) 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.) *shrug* > + > + 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. > + 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'). Max
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 >> updating > > I 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] = v > > Could 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.. > > 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.
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 >>> updating >> >> I 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] = v >> >> Could 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. > >
19.07.2021 15:58, Vladimir Sementsov-Ogievskiy wrote: >>>> >>> >>> Could 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 This way you also drop compression_type if test specify it. I think we shouldn't touch test specified options. Let it clearly fail instead. We only want to ignore compression_type in IMGOPTS when create non-qcow2 image. I think I'll drop checking for compat: the only user for this check ic 065 and it's simpler to explicitly set compression_type in it even for compat=0.10 cases.
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' + for k, v in imgopts.items(): + if k in opts: + continue + if k == 'compression_type' and (compat or parsed.f != 'qcow2'): + continue + opts[k] = v + + result = ['create'] + if parsed.f is not None: + result += ['-f', parsed.f] + 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:
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 updating 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. 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(-)