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

[Vladimir Sementsov-Ogievskiy]

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
> index f0db9f4..d9c8513 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -129,19 +129,21 @@ class Qcow2BitmapExt(Qcow2Struct):
>           ('u64', '{:#x}', 'bitmap_directory_offset')
>       )
>   
> -    def __init__(self, fd):
> +    def __init__(self, fd, cluster_size):
>           super().__init__(fd=fd)
>           pad = (struct.calcsize(self.fmt) + 7) & ~7
>           if pad:
>               fd.seek(pad, 1)
>           position = fd.tell()
> +        self.cluster_size = cluster_size
>           self.read_bitmap_directory(fd)
>           fd.seek(position)
>   
>       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.

>   
>       def dump(self):
>           super().dump()
> @@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>           ('u32', '{}', 'extra_data_size')
>       )
>   
> -    def __init__(self, fd):
> +    def __init__(self, fd, cluster_size):
>           super().__init__(fd=fd)
> +        self.cluster_size = cluster_size
>           # Seek relative to the current position in the file
>           fd.seek(self.extra_data_size, 1)
>           bitmap_name = fd.read(self.name_size)
> @@ -203,11 +206,13 @@ class QcowHeaderExtension(Qcow2Struct):
>           # then padding to next multiply of 8
>       )
>   
> -    def __init__(self, magic=None, length=None, data=None, fd=None):
> +    def __init__(self, magic=None, length=None, data=None, fd=None,
> +                 cluster_size=None):
>           """
>           Support both loading from fd and creation from user data.
>           For fd-based creation current position in a file will be used to read
>           the data.
> +        The cluster_size value may be obtained by dependent structures.
>   
>           This should be somehow refactored and functionality should be moved to
>           superclass (to allow creation of any qcow2 struct), but then, fields
> @@ -230,7 +235,7 @@ class QcowHeaderExtension(Qcow2Struct):
>               assert all(v is None for v in (magic, length, data))
>               super().__init__(fd=fd)
>               if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
> -                self.obj = Qcow2BitmapExt(fd=fd)
> +                self.obj = Qcow2BitmapExt(fd=fd, cluster_size=cluster_size)
>               else:
>                   padded = (self.length + 7) & ~7
>                   self.data = fd.read(padded)
> @@ -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:


--
Best regards,
Vladimir