Re: [Qemu-devel] [PATCH v3 7/7] iotests: add iotest 236 for testing bitmap merge

[Vladimir Sementsov-Ogievskiy]

15.12.2018 2:15, John Snow wrote:
> New interface, new smoke test.

don't forget to add s-o-b here and in previous patch :)


Feel, that I do too much nitpicking.
Actually test is good and works for me,
so, with at least
qemu_img_pipe -> qemu_img or qemu_img_create
and added newline at the end of group file, as git recommends:

Reviewed-by: Vladimir Sementsov-Ogievskiy 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 7/7] iotests: add iotest 236 for testing bitmap merge

[Vladimir Sementsov-Ogievskiy]

18.12.2018 0:32, John Snow wrote:
> 
> 
> On 12/17/18 10:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.12.2018 2:15, John Snow wrote:
>>> New interface, new smoke test.
>>> ---
>>>tests/qemu-iotests/236 | 124 +++
>>>tests/qemu-iotests/236.out | 200 +
>>>tests/qemu-iotests/group   |   1 +
>>>3 files changed, 325 insertions(+)
>>>create mode 100755 tests/qemu-iotests/236
>>>create mode 100644 tests/qemu-iotests/236.out
>>>
>>> diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
>>> new file mode 100755
>>> index 00..edf16c4ab1
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/236
>>> @@ -0,0 +1,124 @@
>>> +#!/usr/bin/env python
>>> +#
>>> +# Test bitmap merges.
>>> +#
>>> +# Copyright (c) 2018 John Snow for Red Hat, Inc.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see .
>>> +#
>>> +# owner=js...@redhat.com
>>> +
>>> +import json
>>> +import iotests
>>> +from iotests import log
>>> +
>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>> +
>>
>> we hardcode patterns, so it's better to hardcode size here too:
>>
>> size = 64 * 1024 * 1024
> 
> If you insist.
> 
>>
>>> +patterns = [("0x5d", "0", "64k"),
>>> +("0xd5", "1M","64k"),
>>> +("0xdc", "32M",   "64k"),
>>> +("0xcd", "0x3ff", "64k")]  # 64M - 64K
>>> +
>>> +overwrite = [("0xab", "0", "64k"), # Full overwrite
>>> + ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
>>> + ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
>>> + ("0xea", "0x3fe", "64k")] # Adjacent-left (64M - 128K)
>>> +
>>> +def query_bitmaps(vm):
>>> +res = vm.qmp("query-block")
>>> +return {device['device']: device['dirty-bitmaps'] for
>>> +device in res['return']}
>>> +
>>> +with iotests.FilePath('img') as img_path, \
>>> + iotests.VM() as vm:
>>> +
>>> +log('--- Preparing image & VM ---\n')
>>> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
>>
>> no needs for qemu_img_pipe, as you don't use its output, qemu_img() is 
>> enough and qemu_img_create() is better,
>> and the best I think is just:
>>
> 
> More cargo cult copy and paste.
> 
>> vm.add_drive_raw('id=drive0,driver=null-co,size=' + str(size))
>>
>> as qcow2 (and real disk in general) is actually unrelated to the test.
>>
> 
> I think I'll leave the image creation in just so that if you run the
> test under different formats it'll actually do something vaguely
> different, just in case.

hm, but you said, supported_fmts=['qcow2']?

I'm ok with leaving image creation too, anyway.

> 
> Actually, it did highlight how weird VPC is again and I changed the rest
> as a result to accommodate image formats that round their disk sizes.
> 
>>
>>> +vm.add_drive(img_path)
>>> +vm.launch()
>>> +
>>> +log('--- Adding preliminary bitmaps A & B ---\n')
>>> +vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA")
>>> +vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")
>>> +
>>> +# Dirties 4 clusters. count=262144
>>> +log('')
>>> +log('--- Emulating writes ---\n')
>>> +for p in patterns:
>>> +cmd = "write -P%s %s %s" % p
>>> +log(cmd)
>>> +log(vm.hmp_qemu_io("drive0", cmd))
>>> +
>>> +log(json.dumps(query_bitmaps(vm), indent=2, separators=(',', ': ')))
>>
>> log can do json.dumps, so it's strange to do it here, and I don't like ',' 
>> separator, as I described
> 
> Because it tries to filter the rich object before it converts, as you've
> noticed. I think I'll take a page from your book and try to fix log() to
> be better.
> 
> As for not liking the ',' separator, it should be identical to your
> approach because it only removes the space when pretty printing is
> enabled. Can you show me what this approach does that you find to be
> ugly and how it's different from your regex approach?

I was wrong, sorry)

But, on the other hand, right-stripping lines in log() is a good thing anyway. 
Or, may be even better (as we
don't have such tests yet), just error out if any trailing whitespaces found 
(and use correct separators for
json.dumps)

> 
> --js
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 7/7] iotests: add iotest 236 for testing bitmap merge

[John Snow]

On 12/17/18 10:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.12.2018 2:15, John Snow wrote:
>> New interface, new smoke test.
>> ---
>>   tests/qemu-iotests/236 | 124 +++
>>   tests/qemu-iotests/236.out | 200 +
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 325 insertions(+)
>>   create mode 100755 tests/qemu-iotests/236
>>   create mode 100644 tests/qemu-iotests/236.out
>>
>> diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
>> new file mode 100755
>> index 00..edf16c4ab1
>> --- /dev/null
>> +++ b/tests/qemu-iotests/236
>> @@ -0,0 +1,124 @@
>> +#!/usr/bin/env python
>> +#
>> +# Test bitmap merges.
>> +#
>> +# Copyright (c) 2018 John Snow for Red Hat, Inc.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see .
>> +#
>> +# owner=js...@redhat.com
>> +
>> +import json
>> +import iotests
>> +from iotests import log
>> +
>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>> +
> 
> we hardcode patterns, so it's better to hardcode size here too:
> 
> size = 64 * 1024 * 1024

If you insist.

> 
>> +patterns = [("0x5d", "0", "64k"),
>> +("0xd5", "1M","64k"),
>> +("0xdc", "32M",   "64k"),
>> +("0xcd", "0x3ff", "64k")]  # 64M - 64K
>> +
>> +overwrite = [("0xab", "0", "64k"), # Full overwrite
>> + ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
>> + ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
>> + ("0xea", "0x3fe", "64k")] # Adjacent-left (64M - 128K)
>> +
>> +def query_bitmaps(vm):
>> +res = vm.qmp("query-block")
>> +return {device['device']: device['dirty-bitmaps'] for
>> +device in res['return']}
>> +
>> +with iotests.FilePath('img') as img_path, \
>> + iotests.VM() as vm:
>> +
>> +log('--- Preparing image & VM ---\n')
>> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
> 
> no needs for qemu_img_pipe, as you don't use its output, qemu_img() is enough 
> and qemu_img_create() is better,
> and the best I think is just:
> 

More cargo cult copy and paste.

> vm.add_drive_raw('id=drive0,driver=null-co,size=' + str(size))
> 
> as qcow2 (and real disk in general) is actually unrelated to the test.
> 

I think I'll leave the image creation in just so that if you run the
test under different formats it'll actually do something vaguely
different, just in case.

Actually, it did highlight how weird VPC is again and I changed the rest
as a result to accommodate image formats that round their disk sizes.

> 
>> +vm.add_drive(img_path)
>> +vm.launch()
>> +
>> +log('--- Adding preliminary bitmaps A & B ---\n')
>> +vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA")
>> +vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")
>> +
>> +# Dirties 4 clusters. count=262144
>> +log('')
>> +log('--- Emulating writes ---\n')
>> +for p in patterns:
>> +cmd = "write -P%s %s %s" % p
>> +log(cmd)
>> +log(vm.hmp_qemu_io("drive0", cmd))
>> +
>> +log(json.dumps(query_bitmaps(vm), indent=2, separators=(',', ': ')))
> 
> log can do json.dumps, so it's strange to do it here, and I don't like ',' 
> separator, as I described

Because it tries to filter the rich object before it converts, as you've
noticed. I think I'll take a page from your book and try to fix log() to
be better.

As for not liking the ',' separator, it should be identical to your
approach because it only removes the space when pretty printing is
enabled. Can you show me what this approach does that you find to be
ugly and how it's different from your regex approach?

--js

Re: [Qemu-devel] [PATCH v3 7/7] iotests: add iotest 236 for testing bitmap merge

[Vladimir Sementsov-Ogievskiy]

15.12.2018 2:15, John Snow wrote:
> New interface, new smoke test.
> ---
>   tests/qemu-iotests/236 | 124 +++
>   tests/qemu-iotests/236.out | 200 +
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 325 insertions(+)
>   create mode 100755 tests/qemu-iotests/236
>   create mode 100644 tests/qemu-iotests/236.out
> 
> diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
> new file mode 100755
> index 00..edf16c4ab1
> --- /dev/null
> +++ b/tests/qemu-iotests/236
> @@ -0,0 +1,124 @@
> +#!/usr/bin/env python
> +#
> +# Test bitmap merges.
> +#
> +# Copyright (c) 2018 John Snow for Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +# owner=js...@redhat.com
> +
> +import json
> +import iotests
> +from iotests import log
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +

we hardcode patterns, so it's better to hardcode size here too:

size = 64 * 1024 * 1024

> +patterns = [("0x5d", "0", "64k"),
> +("0xd5", "1M","64k"),
> +("0xdc", "32M",   "64k"),
> +("0xcd", "0x3ff", "64k")]  # 64M - 64K
> +
> +overwrite = [("0xab", "0", "64k"), # Full overwrite
> + ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
> + ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
> + ("0xea", "0x3fe", "64k")] # Adjacent-left (64M - 128K)
> +
> +def query_bitmaps(vm):
> +res = vm.qmp("query-block")
> +return {device['device']: device['dirty-bitmaps'] for
> +device in res['return']}
> +
> +with iotests.FilePath('img') as img_path, \
> + iotests.VM() as vm:
> +
> +log('--- Preparing image & VM ---\n')
> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')

no needs for qemu_img_pipe, as you don't use its output, qemu_img() is enough 
and qemu_img_create() is better,
and the best I think is just:

vm.add_drive_raw('id=drive0,driver=null-co,size=' + str(size))

as qcow2 (and real disk in general) is actually unrelated to the test.


> +vm.add_drive(img_path)
> +vm.launch()
> +
> +log('--- Adding preliminary bitmaps A & B ---\n')
> +vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA")
> +vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")
> +
> +# Dirties 4 clusters. count=262144
> +log('')
> +log('--- Emulating writes ---\n')
> +for p in patterns:
> +cmd = "write -P%s %s %s" % p
> +log(cmd)
> +log(vm.hmp_qemu_io("drive0", cmd))
> +
> +log(json.dumps(query_bitmaps(vm), indent=2, separators=(',', ': ')))

log can do json.dumps, so it's strange to do it here, and I don't like ',' 
separator, as I described



-- 
Best regards,
Vladimir