Re: [PATCH v8 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method

[Vladimir Sementsov-Ogievskiy]

03.07.2020 16:13, Andrey Shinkevich wrote:
> There are two ways to initialize a class derived from Qcow2Struct:
> 1. Pass a block of binary data to the constructor.
> 2. Pass the file descriptor to allow reading the file from constructor.
> Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
> support a scattered reading in the initialization chain.
> The implementation comes with the patch that follows.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2_format.py | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/qcow2_format.py 
> b/tests/qemu-iotests/qcow2_format.py
> index 2f3681b..1435e34 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -63,7 +63,8 @@ class Qcow2StructMeta(type):
>   
>   class Qcow2Struct(metaclass=Qcow2StructMeta):
>   
> -    """Qcow2Struct: base class for qcow2 data structures
> +    """
> +    Qcow2Struct: base class for qcow2 data structures

Unrelated chunk. And why?

>   
>       Successors should define fields class variable, which is: list of tuples,
>       each of three elements:
> @@ -113,6 +114,9 @@ class Qcow2BitmapExt(Qcow2Struct):
>           ('u64', '{:#x}', 'bitmap_directory_offset')
>       )
>   
> +    def __init__(self, fd):
> +        super().__init__(fd=fd)

this does nothing. We inherit the __init__ of super class, no need to define it 
just to call same __init__.

> +
>   
>   QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
>   
> @@ -173,7 +177,13 @@ class QcowHeaderExtension(Qcow2Struct):
>           self.data_str = data_str
>   
>           if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
> -            self.obj = Qcow2BitmapExt(data=self.data)
> +            assert fd is not None
> +            position = fd.tell()
> +            # Step back to reread data

This definitely shows that we are doing something wrong

> +            padded = (self.length + 7) & ~7
> +            fd.seek(-padded, 1)
> +            self.obj = Qcow2BitmapExt(fd=fd)
> +            fd.seek(position)
>           else:
>               self.obj = None
>   


--
Best regards,
Vladimir