On 10.07.19 19:52, John Snow wrote:
> 
> 
> 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?

It’s OK with the comment.
Although it needs a kwargs.setdefault('job_id', job_id), too.

(Hm.  Shouldn’t 'auto-finalize' and 'job-id' work just fine?  It’s not
like the QMP scripts swap - and _.  They just replace _  by -.)

Max

>> The rest looks good to me:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
> 
> Thanks for reviewing, as always!
>