[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/8] iotests/257: Refactor backup helpers
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 3/8] iotests/257: Refactor backup helpers |
Date: |
Wed, 10 Jul 2019 18:04:58 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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.)
The rest looks good to me:
Reviewed-by: Max Reitz <address@hidden>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [PATCH 4/8] block/backup: hoist bitmap check into QMP interface, (continued)
[Qemu-block] [PATCH 5/8] iotests/257: test API failures, John Snow, 2019/07/09
[Qemu-block] [PATCH 7/8] block/backup: support bitmap sync modes for non-bitmap backups, John Snow, 2019/07/09
[Qemu-block] [PATCH 3/8] iotests/257: Refactor backup helpers, John Snow, 2019/07/09
- Re: [Qemu-block] [PATCH 3/8] iotests/257: Refactor backup helpers,
Max Reitz <=
[Qemu-block] [PATCH 8/8] iotests/257: test traditional sync modes, John Snow, 2019/07/09