Re: [PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'

[Vladimir Sementsov-Ogievskiy]

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.


--
Best regards,
Vladimir