[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/8] iotests/257: Refactor backup helpers
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 3/8] iotests/257: Refactor backup helpers |
Date: |
Wed, 10 Jul 2019 22:17:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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 <address@hidden>
>>> ---
>>> 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 <address@hidden>
>>
>
> Thanks for reviewing, as always!
>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 6/8] block/backup: issue progress updates for skipped regions, (continued)
[Qemu-devel] [PATCH 5/8] iotests/257: test API failures, John Snow, 2019/07/09
[Qemu-devel] [PATCH 3/8] iotests/257: Refactor backup helpers, John Snow, 2019/07/09
[Qemu-devel] [PATCH 8/8] iotests/257: test traditional sync modes, John Snow, 2019/07/09