Re: [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

On 16.07.2020 12:26, Vladimir Sementsov-Ogievskiy wrote:
> 14.07.2020 00:36, Andrey Shinkevich wrote:
>> The cluster size of an image is the QcowHeader class member and may be
>> obtained by dependent extension structures such as Qcow2BitmapExt for
>> further bitmap table details print.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/qcow2_format.py | 18 +++++++++++-------
>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/qcow2_format.py 
>> b/tests/qemu-iotests/qcow2_format.py
>>
...
>>         def read_bitmap_directory(self, fd):
>>           fd.seek(self.bitmap_directory_offset)
>>           self.bitmap_directory = \
>> -            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
>> +            [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>> +             for _ in range(self.nb_bitmaps)]
>
> Better to inline the bitmap directory loading code into __init__:
>
> Benefits:
>  1. Less code. read_bitmap_directory() is very small, used only in 
> __init__ and just not needed as a separate method. __init__ is very 
> small and simple too, so it's not a problem.
>  2. no need of extra self.cluster_size variable (you can use 
> cluster_size parameter directly)
>  3. keep all fd.seek logic in one method
>
> but it's not about this patch.
>

The idea behind the read_bitmap_directory() method was an encapsulation 
of reading the optional parameter. So, we can be flexible in future. 
Sure, there are prones and cons for that in the current implementation.


Andrey


>>         def dump(self):
>>           super().dump()
>> @@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>>
...
>> @@ -244,7 +249,6 @@ class QcowHeaderExtension(Qcow2Struct):
>>                       data_str = '<binary>'
>>                   self.data_str = data_str
>>   -
>
> Unrelated style-fixing chunk, please don't do so.
>
> with it dropped:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>>       def dump(self):
>>           super().dump()
>>   @@ -316,7 +320,7 @@ class QcowHeader(Qcow2Struct):
>>               end = self.cluster_size
>>             while fd.tell() < end:
>> -            ext = QcowHeaderExtension(fd=fd)
>> +            ext = QcowHeaderExtension(fd=fd, 
>> cluster_size=self.cluster_size)
>>               if ext.magic == 0:
>>                   break
>>               else:
>>
>
>