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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6123 bytes --]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]