Re: [Qemu-block] [PATCH v9 07/13] iotests: prepare 124 and 257 bitmap querying for backup-top filter

[Vladimir Sementsov-Ogievskiy]

28.08.2019 19:40, Max Reitz wrote:
> On 26.08.19 18:13, Vladimir Sementsov-Ogievskiy wrote:
>> After backup-top filter appearing it's not possible to see dirty
>> bitmaps in top node, so use node-name instead.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   tests/qemu-iotests/124        |   3 +-
>>   tests/qemu-iotests/257        |  39 +---
>>   tests/qemu-iotests/257.out    | 364 +++++++++++++---------------------
>>   tests/qemu-iotests/iotests.py |  22 ++
>>   4 files changed, 173 insertions(+), 255 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 3440f54781..8b6024769c 100755
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -749,8 +749,7 @@ class 
>> TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
>>   
>>           # Bitmap Status Check
>>           query = self.vm.qmp('query-block')
>> -        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
>> -               if bmap.get('name') == bitmap.name][0]
>> +        ret = self.vm.get_bitmap(None, drive0['id'], bitmap.name)
>>           self.assert_qmp(ret, 'count', 458752)
>>           self.assert_qmp(ret, 'granularity', 65536)
>>           self.assert_qmp(ret, 'status', 'frozen')
> 
> I see a couple more instances of querying the bitmaps through
> query-block here.  Wouldn’t it make sense to replace them all with
> get_bitmap()?
> 
> [...]
> 
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 84438e837c..9381964d9f 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -643,6 +643,28 @@ class VM(qtest.QEMUQtestMachine):
>>                   return x
>>           return None
>>   
>> +    def query_bitmaps(self):
>> +        res = self.qmp("query-named-block-nodes")
>> +        return {"bitmaps": {device['node-name']: device['dirty-bitmaps']
>> +                                for device in res['return']
>> +                                    if 'dirty-bitmaps' in device}}
> 
> I’d leave the wrapping in {"bitmaps": x} to the callers, if they need it.
> 
>> +
>> +    def get_bitmap(self, bitmaps, node_name, name, recording=None):
>> +        """
>> +        get a specific bitmap from the object returned by query_bitmaps.
>> +        :param recording: If specified, filter results by the specified 
>> value.
>> +        """
>> +        if bitmaps is None:
>> +            bitmaps = self.query_bitmaps()
>> +
>> +        for bitmap in bitmaps['bitmaps'][node_name]:
>> +            if bitmap.get('name', '') == name:
>> +                if recording is None:
>> +                    return bitmap
>> +                elif bitmap.get('recording') == recording:
>> +                    return bitmap
> 
> Maybe add a “break” or “return None” here?
> 
> (Yes, yes, you just moved existing code.  Still.)
> 

No, as we may have several unnamed bitmaps, which should be selected by 
"recording"..

> 
>> +        return None
>> +
>>   
>>   index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>>   
>>
> 
> 


-- 
Best regards,
Vladimir