diff mbox series

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

Message ID 1594676203-436999-7-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series iotests: Dump QCOW2 dirty bitmaps metadata | expand

Commit Message

Andrey Shinkevich July 13, 2020, 9:36 p.m. UTC 
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(-)

Comments

Vladimir Sementsov-Ogievskiy July 16, 2020, 9:26 a.m. UTC | #1 
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:
>
Andrey Shinkevich July 17, 2020, 7:18 a.m. UTC | #2 
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:
>>
>
>
diff mbox series

Patch

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)]
 
     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
 
-
     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: