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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2209 bytes --]

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

The rest looks good to me:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]