Re: [Qemu-devel] [PATCH v2 7/7] iotests: add iotest 236 for testing bitmap merge

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> New interface, new smoke test.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>   tests/qemu-iotests/236     | 123 +++++++++++++++++
>   tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 389 insertions(+)
>   create mode 100755 tests/qemu-iotests/236
>   create mode 100644 tests/qemu-iotests/236.out
> 
> diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
> new file mode 100755
> index 0000000000..3d162e967b
> --- /dev/null
> +++ b/tests/qemu-iotests/236
> @@ -0,0 +1,123 @@
> +#!/usr/bin/env python
> +#
> +# Test bitmap merges.
> +#
> +# Copyright (c) 2018 John Snow for Red Hat, Inc.
> +#
> +# 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/>.
> +#
> +# address@hidden
> +
> +import sys
> +import os
> +import iotests
> +from iotests import log
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
> 'scripts'))
> +from qemu import QEMUMachine

unused, with previous line and, therefore, os and sys modules)

> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +patterns = [("0x5d", "0",         "64k"),
> +            ("0xd5", "1M",        "64k"),
> +            ("0xdc", "32M",       "64k"),
> +            ("0xcd", "0x3ff0000", "64k")]  # 64M - 64K
> +
> +overwrite = [("0xab", "0",         "64k"), # Full overwrite
> +             ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
> +             ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
> +             ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K)
> +
> +with iotests.FilePath('img') as img_path, \
> +     iotests.VM() as vm:
> +
> +    log('--- Preparing image & VM ---\n')
> +    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')

hm, actually null device is enough here.

> +    vm.add_drive(img_path)
> +    vm.launch()
> +
> +    log('--- Adding preliminary bitmaps A & B ---\n')
> +    vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA")
> +    vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")
> +
> +    # Dirties 4 clusters. count=262144 > +    log('')
> +    log('--- Emulating writes ---\n')
> +    for p in patterns:
> +        cmd = "write -P%s %s %s" % p
> +        log(cmd)
> +        log(vm.hmp_qemu_io("drive0", cmd))
> +
> +    vm.qmp_log("query-block", indent=2,
> +               filters=[iotests.filter_generated_node_ids,
> +                        iotests.filter_testfiles])

I'm against. query-block prints a lot of unrelated things, which may change from
version to version (for example, Andrey is now adding bitmap information to 
qcow2
format-specific info), then, backported test may fail for previous versions (or 
just
different config) because of something absolutely unrelated to bitmaps.

I think, it should be shortened to result[0]['device'], 
result[0]['dirty-bitmaps']

And, I think, any way it would be good to create a separate helper for printing
current state of bitmaps (which may output their sha256 too)

> +
> +    log('')
> +    log('--- Disabling B & Adding C ---\n')

why not just
log('\n--- Disabling B & Adding C ---\n')

[...]

Otherwise, the test looks fine for me


-- 
Best regards,
Vladimir