Re: [PATCH v12 01/11] iotests: add test for QCOW2 header dump

[Vladimir Sementsov-Ogievskiy]

30.07.2020 17:15, Andrey Shinkevich wrote:
> The simple script creates a QCOW2 image and fills it with some data.
> Two bitmaps are created as well. Then the script reads the image header
> with extensions from the disk by running the script qcow2.py and dumps
> the information to the output. Other entities, such as snapshots, may
> be added to the test later.
>
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/303     | 59 ++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/303.out | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 124 insertions(+)
>   create mode 100755 tests/qemu-iotests/303
>   create mode 100644 tests/qemu-iotests/303.out
>
> diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
> new file mode 100755
> index 0000000..3c7a611
> --- /dev/null
> +++ b/tests/qemu-iotests/303
> @@ -0,0 +1,59 @@
> +#!/usr/bin/env python3
> +#
> +# Test for dumping of qcow2 image metadata
> +#
> +# Copyright (c) 2020 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import iotests
> +import subprocess
> +from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
> +
> +iotests.script_initialize(supported_fmts=['qcow2'])
> +
> +disk = file_path('disk')
> +chunk = 1024 * 1024
> +
> +
> +def create_bitmap(bitmap_number, disabled):
> +    granularity = 1 << (14 + bitmap_number)
> +    bitmap_name = 'bitmap-' + str(bitmap_number)
> +    vm = iotests.VM().add_drive(disk)
> +    vm.launch()
> +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
> +               granularity=granularity, persistent=True, disabled=disabled)
> +    vm.shutdown()
> +
> +
> +def write_to_disk(offset, size):
> +    write = f'write {offset} {size}'
> +    log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
> +
> +
> +def add_bitmap(num, begin, end, disabled):
> +    log(f'Add bitmap {num}')
> +    create_bitmap(num, disabled)
> +    for i in range(begin, end):
> +        write_to_disk((i-1) * chunk, chunk)

a bit unusual to count chunks starting from "1"..

also, any difference with just

write_to_disk((i-1) * chunk, (end-begin) * chunk)

?

> +    log('')
> +
> +
> +qemu_img_create('-f', iotests.imgfmt, disk, '10M')
> +
> +add_bitmap(1, 1, 7, False)
> +add_bitmap(2, 7, 9, True)
> +dump = ['qcow2.py', f'{disk}', 'dump-header']

No reason to put disk into f-string, it's a string anyway: f'{disk}' is equal 
to just disk.

> +subprocess.run(dump)


And you may use just

   subprocess.run(['qcow2.py', disk, 'dump-header'])

without additional variable.


Still, I'm OK with it as is:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


--
Best regards,
Vladimir