Re: [PATCH 04/67] iotests.py: create_test_image, remove_test_image

[John Snow <jsnow@redhat.com>]

On 10/1/19 3:46 PM, Max Reitz wrote:
> Python tests should use these two new functions instead of
> qemu_img('create', ...) + os.remove(), so that user-supplied image
> options are interpreted and handled correctly.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 56 +++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b5ea424de4..fce1ab04c9 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -122,6 +122,62 @@ def qemu_img_create(*args):
>  
>      return qemu_img(*args)
>  
> +def create_test_image(filename, size=None, fmt=imgfmt, opts=[],
> +                      backing_file=None, backing_fmt=None,
> +                      objects=[], unsafe=False):

Python! It's the language that everybody loves and can do no wrong!

Ah, wait, no, maybe the opposite.

You want this:

(..., opts=None, ...):
    opts = opts or []

because, unfortunately, default parameters are bound at definition time
and not at call time, so the default list here is like a static local.

> +    if fmt == imgfmt:
> +        # Only use imgopts for the default format
> +        opts = imgopts + opts
> +
> +    for i, opt in enumerate(opts):
> +        if '$TEST_IMG' in opt:
> +            opts[i] = opt.replace('$TEST_IMG', filename)
> +
> +    # default luks support
> +    if fmt == 'luks':
> +        if not any('key-secret' in opt for opt in opts):

You can write "if not 'key-secret' in opts"

> +            opts.append(luks_default_key_secret_opt)

And here we might modify that default list.

> +        objects.append(luks_default_secret_object)
> +
> +    args = ['create', '-f', fmt]
> +
> +    if len(opts) > 0:
> +        args += ['-o', ','.join(opts)]
> +
> +    if backing_file is not None:
> +        args += ['-b', backing_file]
> +
> +    if backing_fmt is not None:
> +        args += ['-F', backing_fmt]
> +
> +    if len(objects) > 0:
> +        # Generate a [['--object', $obj], [...], ...] list and flatten it
> +        args += [arg for objarg in (['--object', obj] for obj in objects) \
> +                     for arg in objarg]

I may have mentioned at one point that I love comprehensions, but
dislike nested comprehensions. At this point, I think it's far simpler
to say:

for obj in objects:
    args.extend(['--object', obj])

or, even shorter:
    args += ['--object', obj]

> +
> +    if unsafe:
> +        args.append('-u')
> +
> +    args.append(filename)
> +    if size is not None:
> +        args.append(str(size))
> +
> +    return qemu_img(*args)
> +
> +# Use this to remove images create with create_test_image in the

created

and you might as well move the # comment to a """docstring""" while
you're here.

> +# default image format (iotests.imgfmt)
> +def remove_test_image(filename):
> +    try:
> +        os.remove(filename)
> +
> +        data_file = next(opt.replace('data_file=', '') \
> +                            .replace('$TEST_IMG', filename) \
> +                         for opt in imgopts if opt.startswith('data_file='))
> +

Learned something today: you can use next() to get the first value from
a generator expression.

> +        os.remove(data_file)

Keep in mind that if the generator expression returns no results, that
next() will throw an exception and we won't make it here. That's ok, but,

> +    except:
> +        pass
> +

The unqualified except doesn't help me know which errors you expected
and which you didn't.

We have a function like this elsewhere in the python directory:

def remove_if_exists(filename):
    try:
        os.remove(filename)
    except FileNotFoundError:
        pass

Can we use that here and remove the try:/except: from this function? It
will require you to change the list search to something like this instead:

remove_if_exists(filename)
for opt in (x for x in imgopts if etc):
    data_file = opt.replace('etc', 'etc')
    remove_if_exists(data_file)

to avoid the exception when you call next().

>  def qemu_img_verbose(*args):
>      '''Run qemu-img without suppressing its output and return the exit code'''
>      exitcode = subprocess.call(qemu_img_args + list(args))
> 

My fussiness with the remove() function is just optional picky stuff,
but the rest matters, I think.

--js