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

[John Snow]

On 12/19/18 2:34 PM, Eric Blake wrote:
> On 12/18/18 7:52 PM, John Snow wrote:
>> New interface, new smoke test.
>> ---
>>   tests/qemu-iotests/236     | 131 ++++++++++++++++++++++++
>>   tests/qemu-iotests/236.out | 198 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 330 insertions(+)
>>   create mode 100755 tests/qemu-iotests/236
>>   create mode 100644 tests/qemu-iotests/236.out
>>
> 
>> +
>> +    log('')
>> +    log('--- Disabling B & Adding C ---\n')
>> +    vm.qmp_log("transaction", indent=2, actions=[
>> +        { "type": "block-dirty-bitmap-disable",
>> +          "data": { "node": "drive0", "name": "bitmapB" }},
>> +        { "type": "block-dirty-bitmap-add",
>> +          "data": { "node": "drive0", "name": "bitmapC",
>> +                    "granularity": granularity }},
>> +        # Purely extraneous, but test that it works:
>> +        { "type": "block-dirty-bitmap-disable",
>> +          "data": { "node": "drive0", "name": "bitmapC" }},
>> +        { "type": "block-dirty-bitmap-enable",
>> +          "data": { "node": "drive0", "name": "bitmapC" }},
>> +    ])
> 
> One other possible addition just before this point:
> 
> qmp_log("transaction", indent=2, actions=[
>         { "type": "block-dirty-bitmap-disable",
>           "data": { "node": "drive0", "name": "bitmapB" }},
>         { "type": "block-dirty-bitmap-add",
>           "data": { "node": "drive0", "name": "bitmapC",
>                     "granularity": granularity }},
>         { "type": "block-dirty-bitmap-remove",
>           "data": { "node": "drive0", "name": "bitmapA" }},
>         { "type": "abort", "data": {}}
>   ])
> 
> to check that we properly undo things on an aborted transaction (B
> should still be enabled, C should not exist, and A should not be damaged).
> 

Good suggestion, but remove is not a transaction item. I'll substitute
for clear, which should showcase the same effects.

>> +    # A and D should be equivalent>> +    # Some formats round the size of 
>> the disk, so don't print the
>> checksums.
>> +    check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> +                     node="drive0", name="bitmapA")['return']['sha256']
>> +    check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> +                     node="drive0", name="bitmapD")['return']['sha256']
>> +    assert(check_a == check_b)
> 
> Image agnostic also means that you avoid an 32- vs. 64-bit platform
> discrepancies (we've had issues in the past where the sum is different
> for some image sizes, because the bitmap is always in terms of 'long's,
> but there is one less 'long' in a 32-bit bitmap for the disk size).
> Makes sense.
> 
> Also, I don't see any tests of block-dirty-bitmap-remove - this would be
> a good point in the test to insert a final cleanup.
> 

OK, done.

> 
>> +++ b/tests/qemu-iotests/group
>> @@ -233,3 +233,4 @@
>>   233 auto quick
>>   234 auto quick migration
>>   235 auto quick
>> +236 auto quick
>> \ No newline at end of file
> 
> Umm - what's that still doing here?
> 
> 
:whistles: