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

[John Snow <jsnow@redhat.com>]

On 6/20/19 3:48 PM, Max Reitz wrote:
> 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 <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.
>>>
>>
>> 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.
> 

I actually really dislike the postfix-if-else too. I prefer C's ternary
there, but when in Rome, etc.

>> 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!!)
> 

ok!!!!!

>>>> +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.)
> 

It turns out that your suggestion is fine. I do agree, though: I like my
unnecessary parentheses a lot.

Python wants:

assert a == b

And will get mad about:

assert (a == b)

And that's just so hard to deal with when working with C-brain.

>> (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
>