On 20.06.19 03:03, John Snow wrote: > Signed-off-by: John Snow <js...@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
signature.asc
Description: OpenPGP digital signature
