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

[Max Reitz]

On 20.06.19 21:08, John Snow wrote:
> 
> 
> On 6/20/19 2:35 PM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  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.
>>
> 
> Thanks!
> 
>> I like that I don’t have to read the reference output but can just grep
>> for “error”.
>>
> 
> Me too!! Actually, doing the math for what to expect and verifying the
> output by hand was becoming a major burden, so partially this test
> infrastructure was my attempt to procedurally verify that the results I
> was seeing were what made sense.
> 
> At the end of it, I felt it was nice to keep it in there.
> 
>> 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().
>>
> 
> It confuses me constantly, but it's really meant to be used for
> iterating over lengths.

I can see the use for range(x), but not for range(a, b).

(At least it’s not Rust, where [a..b] is [a, b), too – it’s enclosed in
square brackets, it should be closed, damnit.)

> This is somewhat of an abuse of it. I always
> test it out in a console first before using it, just in case.
> 
>> (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.
>>
> 
> Either is fine, I think. In this case it affords us more room for the
> commentary on the bit ranges. Maybe it's not necessary, but at least
> personally I get woozy looking at the bit patterns.

Oh, no, no, I just meant the final closing ”]” of GROUPS.

(I did wonder about why you didn’t place every PatternGroups closing ])
on a separate line, too, but I decided not to say anything, because it
looks Python-y this way.  But you’re right, this gives a nice excuse why
to put more space between the patterns and the comments, which helps.)

>>> +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.
>>
> 
> Mostly just because I already wrote this for the last test, and we
> already test incremental backups the other way. I figured this would
> just be nice for code coverage purposes, and also because using the
> blockdev interfaces exclusively does tend to reveal little gotchas
> everywhere.
> 
> I also kind of want to refactor a lot of my tests and share some of the
> common code. I was tinkering with the idea of adding some common objects
> to iotests, like "Drive" "Bitmap" and "Backup".
> 
> That's why there's a kind of superfluous "Drive" object here.
> 
>>> +
>>> +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?
>>
>> [...]
>>
> 
> Sorry. It's a very functional-esque way of processing iterables.
I’m not blaming the basic idea, I’m blaming the syntax.  In fact, I’m
blaming exactly that it isn’t literally functional because there are no
functions here (but .get() and []).  I would like to have functions
because function names would tell me what’s going on.

I can never remember what {:} means (why do they use such nice words
everywhere else, like “or”, “for”, or “in”, and then they do that?).
And I find it weird that postfix-for can introduce variables after they
are used.  That may be the way I would read it aloud, but that’s
definitely not the way I *think*.

> I've been doing a lot of FP stuff lately and that skews what I find
> readable...
> 
> It's a dict comprehension of the form:
> 
> {key: value for atom in iterable}

Ah.  Thanks.  I thought it was some filter where it would only return
elements where 'device' or 'qdev' is set.  So that seemed completely
stupid to me, to have the iteration in the end, but the filter in the
beginning.

> Key here is either the device or qdev name,
> the value is the 'dirty-bitmaps' field of the atom, and
> res['return'] is the iterable.
> 
> Effectively it turns a list of devices into a dict keyed by the device
> name, and the only field (if any) was its dirty-bitmap object.

Why can’t they write it like normal human beings.  Like

Hash[res['return'].map { |device| [device['device'] || device['qdev'],
                                   device['dirty-bitmaps'] or {}]}]

By the way, this is the reason why you’ll always see me using map() and
filter() and then someone saying that there is a more Python-y way of
doing themes, namely postfix-for.  I hate postfix-for.  I also hate
postfix-if-else, by the way, but I felt like I didn’t want to go there.

> However, in explaining this I do notice I have a bug -- the default
> value for the bitmap object ought to be {}, not [].
> 
> This is code that should become common testing code too, as I've
> re-written it a few times now...
> 
>>> +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']?
>>
>> [...]
>>
> 
> I forget that exists. If you prefer that, OK.

Well, it is shorter and more optimal!!!  (Saves two conversions to FP,
then an FP division, and then one conversion back to integer!!)

>>> +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.)
>>
>> [...]
>>
> 
> Oh, good catch. Alright.
> 
>>> +        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.
>>
>> [...]
>>
> 
> OK; I might need to make a pylintrc file to allow that style. Python is
> VERY aggressively tuned to omitting parentheses.

It seems to me more and more like Python is very aggressively tuned to
what I find difficult to read.

(You’re also still free to write “cancel = failure == 'simulated'”.  I
wouldn’t write that in C, but well.)

> (I do actually try to run pylint on my python patches now to make sure I
> am targeting SOME kind of style. I realize this is not standardized in
> this project.)

Sorry for becoming very grumpy here (can’t help myself), but why would I
run it when apparently Python has just bad opinions about what’s
readable and what isn’t.

Max

signature.asc
Description: OpenPGP digital signature