Re: [Qemu-devel] [PATCH 3/8] iotests/257: Refactor backup helpers

[John Snow <jsnow@redhat.com>]

On 7/10/19 12:04 PM, Max Reitz wrote:
> On 10.07.19 03:05, John Snow wrote:
>> This test needs support for non-bitmap backups and missing or
>> unspecified bitmap sync modes, so rewrite the helpers to be a little
>> more generic.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/257     |  46 +++++----
>>  tests/qemu-iotests/257.out | 192 ++++++++++++++++++-------------------
>>  2 files changed, 124 insertions(+), 114 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>> index 2ff4aa8695..2eb4f26c28 100755
>> --- a/tests/qemu-iotests/257
>> +++ b/tests/qemu-iotests/257
> 
> [...]
> 
>> -def bitmap_backup(drive, n, filepath, bitmap, bitmap_mode):
>> -    log("--- Bitmap Backup #{:d} ---\n".format(n))
>> -    target_id = "bitmap_target_{:d}".format(n)
>> -    job_id = "bitmap_backup_{:d}".format(n)
>> +def backup(drive, n, filepath, bitmap, bitmap_mode, sync='bitmap'):
>> +    log("--- Test Backup #{:d} ---\n".format(n))
>> +    target_id = "backup_target_{:d}".format(n)
>> +    job_id = "backup_{:d}".format(n)
>>      target_drive = Drive(filepath, vm=drive.vm)
>>  
>>      target_drive.create_target(target_id, drive.fmt, drive.size)
>> -    drive.vm.qmp_log("blockdev-backup", job_id=job_id, device=drive.name,
>> -                     target=target_id, sync="bitmap",
>> -                     bitmap_mode=bitmap_mode,
>> -                     bitmap=bitmap,
>> -                     auto_finalize=False)
>> +
>> +    kwargs = {
>> +        'job_id': job_id,
>> +        'auto_finalize': False,
>> +        'bitmap': bitmap,
>> +        'bitmap_mode': bitmap_mode,
>> +    }
>> +    kwargs = {key: val for key, val in kwargs.items() if val is not None}
> 
> I suppose this is to remove items that are None?
> 
> Very cute, but why not just
> 
>   kwargs = {
>     'job_id': job_id,
>     'auto_finalize': False,
>   }
>   if bitmap is not None:
>     kwargs['bitmap'] = bitmap
>     kwargs['bitmap_mode'] = bitmap_mode
> 
> Exactly the same number of lines, but immediately makes it clear what’s
> going on.  Not as cute, I admit.
> 
> (Yes, I am indeed actively trying to train you not to write cute code.)
> 

It sneaks in. I genuinely struggle with understanding what other people
will find readable; I have an authentically hard time reviewing other
people's patches too. I'm earnestly not sure how I can help improve
this, but I would like to.

I wasn't sure what the easiest way to avoid sending the "None" over the
wire was, so I went with a general thing, but yes: it's because bitmap
and bitmap_mode are set to None sometimes and I need to omit such keys.

In this case, though, I do test bitmap and bitmap_mode separately, so
for the purposes of testing intentionally bad combinations you do need:

if bitmap is not None:
    kwargs['bitmap'] = bitmap
if bitmap_mode is not None:
    kwargs['bitmap_mode'] = bitmap_mode

And I just looked at this and it did not spark joy; so I went with a
generic filter to remove nulled keys. I admit it's /slightly/ cute and
not immediately obvious why it needs to be done.


This is even cuter, so maybe I am traveling in the wrong direction:

def backup(drive, n, filepath, sync, **kwargs):
    log("--- Test Backup #{:d} ---\n".format(n))
    target_id = "backup_target_{:d}".format(n)
    job_id = "backup_{:d}".format(n)
    target_drive = Drive(filepath, vm=drive.vm)

    target_drive.create_target(target_id, drive.fmt, drive.size)
    kwargs.setdefault('auto_finalize', False)
    # Strip any arguments explicitly nulled by the caller:
    kwargs = {key: val for key, val in kwargs.items()
              if val is not None}
    blockdev_backup(drive.vm, drive.name, target_id, sync, **kwargs)
    return job_id

It's quite a bit shorter and also makes backup() more flexible by
omitting the bitmap and bitmap_mode arguments entirely, allowing the
caller to override the auto_finalize default, etc. In this permutation,
we don't know the full extent of kwargs so it makes sense to generically
filter it.

Manually conditionally setting arguments is probably also fine.
Do you still have a preference for the more static approach?

> The rest looks good to me:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

Thanks for reviewing, as always!