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