Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups

From: Max Reitz <mreitz@redhat.com>
To: John Snow <jsnow@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	vsementsov@virtuozzo.com, Wen Congyang <wencongyang2@huawei.com>,
	Xie Changlong <xiechanglong.d@gmail.com>,
	Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 11/12] iotests: add test 257 for bitmap-mode backups
Date: Thu, 20 Jun 2019 20:35:18 +0200	[thread overview]
Message-ID: <4f98c07a-cc3a-f249-ba62-b8a98a47ab53@redhat.com> (raw)
In-Reply-To: <20190620010356.19164-12-jsnow@redhat.com>

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

On 20.06.19 03:03, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/257     |  412 +++++++
>  tests/qemu-iotests/257.out | 2199 ++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |    1 +
>  3 files changed, 2612 insertions(+)
>  create mode 100755 tests/qemu-iotests/257
>  create mode 100644 tests/qemu-iotests/257.out

This test is actually quite nicely written.

I like that I don’t have to read the reference output but can just grep
for “error”.

Only minor notes below.

> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
> new file mode 100755
> index 0000000000..5f7f388504
> --- /dev/null
> +++ b/tests/qemu-iotests/257

[...]

> +class PatternGroup:
> +    """Grouping of Pattern objects. Initialize with an iterable of Patterns."""
> +    def __init__(self, patterns):
> +        self.patterns = patterns
> +
> +    def bits(self, granularity):
> +        """Calculate the unique bits dirtied by this pattern grouping"""
> +        res = set()
> +        for pattern in self.patterns:
> +            lower = math.floor(pattern.offset / granularity)
> +            upper = math.floor((pattern.offset + pattern.size - 1) / granularity)
> +            res = res | set(range(lower, upper + 1))

Why you’d do floor((x - 1) / y) + 1 has confused me quite a while.
Until I realized that oh yeah, Python’s range() is a right-open
interval.  I don’t like Python’s range().

(Yes, you’re right, this is better to read than just ceil(x / y).
Because it reminds people like me that range() is weird.)

> +        return res
> +
> +GROUPS = [
> +    PatternGroup([
> +        # Batch 0: 4 clusters
> +        mkpattern('0x49', 0x0000000),
> +        mkpattern('0x6c', 0x0100000),   # 1M
> +        mkpattern('0x6f', 0x2000000),   # 32M
> +        mkpattern('0x76', 0x3ff0000)]), # 64M - 64K
> +    PatternGroup([
> +        # Batch 1: 6 clusters (3 new)
> +        mkpattern('0x65', 0x0000000),   # Full overwrite
> +        mkpattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
> +        mkpattern('0x72', 0x2008000),   # Partial-right (32M+32K)
> +        mkpattern('0x69', 0x3fe0000)]), # Adjacent-left (64M - 128K)
> +    PatternGroup([
> +        # Batch 2: 7 clusters (3 new)
> +        mkpattern('0x74', 0x0010000),   # Adjacent-right
> +        mkpattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
> +        mkpattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
> +        mkpattern('0x67', 0x3fe0000,
> +                  2*GRANULARITY)]),     # Overwrite [(64M-128K)-64M)
> +    PatternGroup([
> +        # Batch 3: 8 clusters (5 new)
> +        # Carefully chosen such that nothing re-dirties the one cluster
> +        # that copies out successfully before failure in Group #1.
> +        mkpattern('0xaa', 0x0010000,
> +                  3*GRANULARITY),       # Overwrite and 2x Adjacent-right
> +        mkpattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
> +        mkpattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
> +        mkpattern('0xdd', 0x3fc0000)]), # New; leaving a gap to the right
> +    ]

I’d place this four spaces to the left.  But maybe placing it here is
proper Python indentation, while moving it to the left would be C
indentation.

> +class Drive:
> +    """Represents, vaguely, a drive attached to a VM.
> +    Includes format, graph, and device information."""
> +
> +    def __init__(self, path, vm=None):
> +        self.path = path
> +        self.vm = vm
> +        self.fmt = None
> +        self.size = None
> +        self.node = None
> +        self.device = None
> +
> +    @property
> +    def name(self):
> +        return self.node or self.device
> +
> +    def img_create(self, fmt, size):
> +        self.fmt = fmt
> +        self.size = size
> +        iotests.qemu_img_create('-f', self.fmt, self.path, str(self.size))
> +
> +    def create_target(self, name, fmt, size):
> +        basename = os.path.basename(self.path)
> +        file_node_name = "file_{}".format(basename)
> +        vm = self.vm
> +
> +        log(vm.command('blockdev-create', job_id='bdc-file-job',
> +                       options={
> +                           'driver': 'file',
> +                           'filename': self.path,
> +                           'size': 0,
> +                       }))
> +        vm.run_job('bdc-file-job')
> +        log(vm.command('blockdev-add', driver='file',
> +                       node_name=file_node_name, filename=self.path))
> +
> +        log(vm.command('blockdev-create', job_id='bdc-fmt-job',
> +                       options={
> +                           'driver': fmt,
> +                           'file': file_node_name,
> +                           'size': size,
> +                       }))
> +        vm.run_job('bdc-fmt-job')
> +        log(vm.command('blockdev-add', driver=fmt,
> +                       node_name=name,
> +                       file=file_node_name))
> +        self.fmt = fmt
> +        self.size = size
> +        self.node = name

It’s cool that you use blockdev-create here, but would it not have been
easier to just use self.img_create() + blockdev-add?

I mean, there’s no point in changing it now, I’m just wondering.

> +
> +def query_bitmaps(vm):
> +    res = vm.qmp("query-block")
> +    return {"bitmaps": {device['device'] or device['qdev']:
> +                        device.get('dirty-bitmaps', []) for
> +                        device in res['return']}}

Python’s just not for me if I find this syntax unintuitive and
confusing, hu?

[...]

> +def bitmap_comparison(bitmap, groups=None, want=0):
> +    """
> +    Print a nice human-readable message checking that this bitmap has as
> +    many bits set as we expect it to.
> +    """
> +    log("= Checking Bitmap {:s} =".format(bitmap.get('name', '(anonymous)')))
> +
> +    if groups:
> +        want = calculate_bits(groups)
> +    have = int(bitmap['count'] / bitmap['granularity'])

Or just bitmap['count'] // bitmap['granularity']?

[...]

> +def test_bitmap_sync(bsync_mode, failure=None):

[...]

> +        log('--- Preparing image & VM ---\n')
> +        drive0 = Drive(img_path, vm=vm)
> +        drive0.img_create(iotests.imgfmt, SIZE)
> +        vm.add_device('virtio-scsi-pci,id=scsi0')

Judging from 238, this should be virtio-scsi-ccw on s390-ccw-virtio.

(This is the reason I cannot give an R-b.)

[...]

> +        vm.run_job(job, auto_dismiss=True, auto_finalize=False,
> +                   pre_finalize=_callback,
> +                   cancel=failure == 'simulated')

I’d prefer “cancel=(failure == 'simulated')”.  (Or spaces around =).

Also in other places elsewhere that are of the form x=y where y contains
spaces.

[...]

> +def main():
> +    for bsync_mode in ("never", "conditional", "always"):
> +        for failure in ("simulated", None):
> +            test_bitmap_sync(bsync_mode, failure)
> +
> +    for bsync_mode in ("never", "conditional", "always"):
> +        test_bitmap_sync(bsync_mode, "intermediate")

Why are these separate?  Couldn’t you just iterate over
("simulated", None, "intermediate")?

Max

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