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 <mre...@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
