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.shinkev...@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 <vsement...@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