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

[John Snow]

On 10/2/19 7:00 AM, Max Reitz wrote:
> 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.
> 

It is by far the WORST thing about Python.

I realize we use this pattern a few places in iotests, but I think it's
also usually where we don't modify the list, so it's actually OK, but
serves as an example of a bad habit.

>>> +    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?
> 

No, I was just mistaken about the shape of the data.
You are looking for 'key-secret=XXX', I was thinking that there was a
token that was really just 'key-secret'.

What you wrote is correct and good and I am wrong and bad.

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

I am sorry I ever mentioned liking Python. I will accept your punishments.

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

I honestly tried to rewrite this a few times because it looks so chunky,
but realized there isn't ... a great way to do this without implying
that you might find more than one result.

You can filter to a new list and assert that the length is one, but
that's not less chunky.

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

It just wasn't clear, because the except is doing the lifting for both
the remove and the finding.

Oh well, it's not really important.

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

I wonder if that's true ... I just don't know what the full set of
errors we might get are. I don't really like exception driven code,
honestly.

"It's wrong to catch ANY exception because you might suppress errors too
broadly."

"It's wrong to be too specific, because you'll miss cases you meant to
catch."

Awful.

Anyway, like I said I was just being fiddly because I found this odd to
read, but really don't have suggestions that are clearly nicer, so ...
carry on.

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

Yeah, that's the one. Don't worry about plucking it out here for this,
just nothing that we do this in a few places. We might want a util
eventually that gets it exactly right.

Or not, because what's "exactly right" anyway. Ah, ah, ah.

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

Nah, Python people LOVE exceptions. They don't like "bare except"
statements, though. I am the weird person in that I like to avoid
exceptions whenever it's elegant and pretty to do so.

I find exceptions as normal control flow to be quite hard to deal with;
but Pythonistas seem to love it.

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