[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 04/67] iotests.py: create_test_image, remove_test_image
From: |
Max Reitz |
Subject: |
Re: [PATCH 04/67] iotests.py: create_test_image, remove_test_image |
Date: |
Wed, 2 Oct 2019 13:00:03 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
On 02.10.19 01:20, John Snow wrote:
>
>
> 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 <address@hidden>
>> ---
>> 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.
OK. Interesting.
I suppose the same goes for @objects, then.
>> + 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"
Oh, that’s recursive?
>> + 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.
I can’t remember but I do remember writing this piece of code, being sad
that there is no .flatten, and wanting everyone to see the monster that
arises.
> 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]
OK, so now you saw it, I’m glad to make the flattening more flattering
to read.
>> +
>> + 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.
I was sad for a bit that Python doesn’t have a find(), but then I
noticed this works as well. (Already used extensively in “iotests: Add
VM.assert_block_path()” from my “block: Fix check_to_replace_node()”
series.)
>> + 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,
I did. If there are no results, it’s good we won’t get here.
This code would be wrong if the next() didn’t throw an exception.
>> + except:
>> + pass
>> +
>
> The unqualified except doesn't help me know which errors you expected
> and which you didn't.
What I’m expecting: FileNotFound, StopIteration.
But the thing is that I feel like maybe removing a file should always
pass, regardless of the exact exception. (I can imagine to be wrong.)
> We have a function like this elsewhere in the python directory:
>
> def remove_if_exists(filename):
> try:
> os.remove(filename)
> except FileNotFoundError:
> pass
We do? I can’t find it. I find a _remove_if_exists in machine.py,
which I’m not sure whether it’s supposed to be used outside, and it
works a bit different, actually (but probably to the same effect).
> 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().
I don’t know why I’d avoid the exception, though.
This is probably because I don’t like pythonic code, again, but I prefer
a next() + exception over a for loop that just iterates once or not at all.
>> 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.
OK. Indeed it does!
Max
signature.asc
Description: OpenPGP digital signature
- [PATCH 02/67] iotests.py: Add @skip_for_imgopts(), (continued)
- [PATCH 06/67] iotests.py: Add image_path(), Max Reitz, 2019/10/01
- [PATCH 08/67] iotests.py: Add filter_json_filename(), Max Reitz, 2019/10/01
- [PATCH 09/67] iotests.py: Add @hide_fields to img_info_log, Max Reitz, 2019/10/01
- [PATCH 05/67] iotests.py: Add ImagePaths, Max Reitz, 2019/10/01
- [PATCH 12/67] iotests/228: Filter json:{} filenames, Max Reitz, 2019/10/01
- [PATCH 13/67] iotests/242: Hide refcount bit information, Max Reitz, 2019/10/01
- [PATCH 07/67] iotests.py: Filter data_file in filter_img_info, Max Reitz, 2019/10/01
- [PATCH 11/67] iotests/224: Filter json:{} from commit command, Max Reitz, 2019/10/01
- [PATCH 14/67] iotests: Use unsupported_imgopts in Python tests, Max Reitz, 2019/10/01