[PATCH v8 00/10] iotests: Dump QCOW2 dirty bitmaps metadata

[PATCH v8 00/10] iotests: Dump QCOW2 dirty bitmaps metadata

[Andrey Shinkevich]

Add dirty bitmap information to QCOW2 metadata dump in the qcow2_format.py.

v8:
  01: Hard refactoring of the v7 code in qcow2_format.py based on Vladimir's
      review. See the thread with the message ID:
      <1591920302-1002219-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey Shinkevich (10):
  qcow2: Fix capitalization of header extension constant.
  qcow2_format.py: make printable data an extension class member
  qcow2_format.py: change Qcow2BitmapExt initialization method
  qcow2_format.py: dump bitmap flags in human readable way.
  qcow2_format.py: Dump bitmap directory information
  qcow2_format.py: pass cluster size to substructures
  qcow2_format.py: Dump bitmap table serialized entries
  qcow2.py: Introduce '-j' key to dump in JSON format
  qcow2_format.py: collect fields to dump in JSON format
  qcow2_format.py: support dumping metadata in JSON format

 block/qcow2.c                      |   2 +-
 docs/interop/qcow2.txt             |   2 +-
 tests/qemu-iotests/291.out         |  90 ++++++++++++++++
 tests/qemu-iotests/qcow2.py        |  19 +++-
 tests/qemu-iotests/qcow2_format.py | 212 ++++++++++++++++++++++++++++++++++---
 5 files changed, 304 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH v8 01/10] qcow2: Fix capitalization of header extension constant.

[Andrey Shinkevich]

Make the capitalization of the hexadecimal numbers consistent for the
QCOW2 header extension constants in docs/interop/qcow2.txt.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c          | 2 +-
 docs/interop/qcow2.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e67..80dfe5f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -66,7 +66,7 @@ typedef struct {
 } QEMU_PACKED QCowExtension;
 
 #define  QCOW2_EXT_MAGIC_END 0
-#define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
+#define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xe2792aca
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index cb72346..f072e27 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -231,7 +231,7 @@ be stored. Each extension has a structure like the following:
 
     Byte  0 -  3:   Header extension type:
                         0x00000000 - End of the header extension area
-                        0xE2792ACA - Backing file format name string
+                        0xe2792aca - Backing file format name string
                         0x6803f857 - Feature name table
                         0x23852875 - Bitmaps extension
                         0x0537be77 - Full disk encryption header pointer
-- 
1.8.3.1

[PATCH v8 02/10] qcow2_format.py: make printable data an extension class member

[Andrey Shinkevich]

Let us differ binary data type from string one for the extension data
variable and keep the string as the QcowHeaderExtension class member.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index cc432e7..2f3681b 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -165,6 +165,13 @@ class QcowHeaderExtension(Qcow2Struct):
             self.data = fd.read(padded)
             assert self.data is not None
 
+        data_str = self.data[:self.length]
+        if all(c in string.printable.encode('ascii') for c in data_str):
+            data_str = f"'{ data_str.decode('ascii') }'"
+        else:
+            data_str = '<binary>'
+        self.data_str = data_str
+
         if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
             self.obj = Qcow2BitmapExt(data=self.data)
         else:
@@ -174,12 +181,7 @@ class QcowHeaderExtension(Qcow2Struct):
         super().dump()
 
         if self.obj is None:
-            data = self.data[:self.length]
-            if all(c in string.printable.encode('ascii') for c in data):
-                data = f"'{ data.decode('ascii') }'"
-            else:
-                data = '<binary>'
-            print(f'{"data":<25} {data}')
+            print(f'{"data":<25} {self.data_str}')
         else:
             self.obj.dump()
 
-- 
1.8.3.1

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

[Andrey Shinkevich]

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
 
     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)
+
 
 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
+            padded = (self.length + 7) & ~7
+            fd.seek(-padded, 1)
+            self.obj = Qcow2BitmapExt(fd=fd)
+            fd.seek(position)
         else:
             self.obj = None
 
-- 
1.8.3.1

[PATCH v8 04/10] qcow2_format.py: dump bitmap flags in human readable way.

[Andrey Shinkevich]

Introduce the class BitmapFlags that parses a bitmap flags mask.

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 1435e34..d8c058d 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -40,6 +40,22 @@ class Flags64(Qcow2Field):
         return str(bits)
 
 
+class BitmapFlags(Qcow2Field):
+
+    flags = {
+        0x1: 'in-use',
+        0x2: 'auto'
+    }
+
+    def __str__(self):
+        bits = []
+        for bit in range(64):
+            flag = self.value & (1 << bit)
+            if flag:
+                bits.append(self.flags.get(flag, '{:#x}'.format(flag)))
+        return f'{self.value:#x} ({bits})'
+
+
 class Enum(Qcow2Field):
 
     def __str__(self):
-- 
1.8.3.1

[PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Andrey Shinkevich]

Read and dump entries from the bitmap directory of QCOW2 image.
It extends the output in the test case #291.

Header extension:
magic                     0x23852875 (Bitmaps)
...

Bitmap name               bitmap-1
bitmap_table_offset       0xf0000
bitmap_table_size         1
flags                     0x2 (['auto'])
type                      1
granularity_bits          16
name_size                 8
extra_data_size           0

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/291.out         | 45 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/qcow2_format.py | 44 +++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index 08bfaaa..53a8eeb 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -33,6 +33,24 @@ reserved32                0
 bitmap_directory_size     0x40
 bitmap_directory_offset   0x510000
 
+Bitmap name               b1
+bitmap_table_offset       0x4e0000
+bitmap_table_size         1
+flags                     0x0 ([])
+type                      1
+granularity_bits          19
+name_size                 2
+extra_data_size           0
+
+Bitmap name               b2
+bitmap_table_offset       0x500000
+bitmap_table_size         1
+flags                     0x2 (['auto'])
+type                      1
+granularity_bits          16
+name_size                 2
+extra_data_size           0
+
 
 === Bitmap preservation not possible to non-qcow2 ===
 
@@ -98,6 +116,33 @@ reserved32                0
 bitmap_directory_size     0x60
 bitmap_directory_offset   0x520000
 
+Bitmap name               b1
+bitmap_table_offset       0x470000
+bitmap_table_size         1
+flags                     0x0 ([])
+type                      1
+granularity_bits          19
+name_size                 2
+extra_data_size           0
+
+Bitmap name               b2
+bitmap_table_offset       0x490000
+bitmap_table_size         1
+flags                     0x2 (['auto'])
+type                      1
+granularity_bits          16
+name_size                 2
+extra_data_size           0
+
+Bitmap name               b0
+bitmap_table_offset       0x510000
+bitmap_table_size         1
+flags                     0x0 ([])
+type                      1
+granularity_bits          16
+name_size                 2
+extra_data_size           0
+
 
 === Check bitmap contents ===
 
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index d8c058d..7c0dc9a 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
 
     def __init__(self, fd):
         super().__init__(fd=fd)
+        self.read_bitmap_directory(fd)
+
+    def read_bitmap_directory(self, fd):
+        fd.seek(self.bitmap_directory_offset)
+        self.bitmap_directory = \
+            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
+
+    def dump(self):
+        super().dump()
+        for entry in self.bitmap_directory:
+            print()
+            entry.dump()
+
+
+class Qcow2BitmapDirEntry(Qcow2Struct):
+
+    fields = (
+        ('u64', '{:#x}', 'bitmap_table_offset'),
+        ('u32', '{}', 'bitmap_table_size'),
+        ('u32', BitmapFlags, 'flags'),
+        ('u8',  '{}', 'type'),
+        ('u8',  '{}', 'granularity_bits'),
+        ('u16', '{}', 'name_size'),
+        ('u32', '{}', 'extra_data_size')
+    )
+
+    def __init__(self, fd):
+        super().__init__(fd=fd)
+        # Seek relative to the current position in the file
+        fd.seek(self.extra_data_size, 1)
+        bitmap_name = fd.read(self.name_size)
+        self.name = bitmap_name.decode('ascii')
+        # Move position to the end of the entry in the directory
+        entry_raw_size = self.bitmap_dir_entry_raw_size()
+        padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
+        fd.seek(padding, 1)
+
+    def bitmap_dir_entry_raw_size(self):
+        return struct.calcsize(self.fmt) + self.name_size + \
+            self.extra_data_size
+
+    def dump(self):
+        print(f'{"Bitmap name":<25} {self.name}')
+        super(Qcow2BitmapDirEntry, self).dump()
 
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
-- 
1.8.3.1

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

[Andrey Shinkevich]

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 | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 7c0dc9a..cfd9646 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -130,14 +130,16 @@ class Qcow2BitmapExt(Qcow2Struct):
         ('u64', '{:#x}', 'bitmap_directory_offset')
     )
 
-    def __init__(self, fd):
+    def __init__(self, fd, cluster_size):
         super().__init__(fd=fd)
+        self.cluster_size = cluster_size
         self.read_bitmap_directory(fd)
 
     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()
@@ -158,8 +160,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)
@@ -199,11 +202,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
@@ -242,7 +247,7 @@ class QcowHeaderExtension(Qcow2Struct):
             # Step back to reread data
             padded = (self.length + 7) & ~7
             fd.seek(-padded, 1)
-            self.obj = Qcow2BitmapExt(fd=fd)
+            self.obj = Qcow2BitmapExt(fd=fd, cluster_size=cluster_size)
             fd.seek(position)
         else:
             self.obj = None
@@ -318,7 +323,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:
-- 
1.8.3.1

[PATCH v8 07/10] qcow2_format.py: Dump bitmap table serialized entries

[Andrey Shinkevich]

Add bitmap table information to the QCOW2 metadata dump.
It extends the output of the test case #291

Bitmap name               bitmap-1
...
Bitmap table   type            offset                   size
0              serialized      4718592                  65536
1              serialized      4294967296               65536
2              serialized      5348033147437056         65536
3              serialized      13792273858822144        65536
4              serialized      4718592                  65536
5              serialized      4294967296               65536
6              serialized      4503608217305088         65536
7              serialized      14073748835532800        65536

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/291.out         | 45 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/qcow2_format.py | 42 +++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index 53a8eeb..9f465c6 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -41,6 +41,15 @@ type                      1
 granularity_bits          19
 name_size                 2
 extra_data_size           0
+Bitmap table   type            offset                   size
+0              serialized      5046272                  65536
+1              all-zeroes      0                        65536
+2              all-zeroes      0                        65536
+3              all-zeroes      0                        65536
+4              all-zeroes      0                        65536
+5              all-zeroes      0                        65536
+6              all-zeroes      0                        65536
+7              all-zeroes      0                        65536
 
 Bitmap name               b2
 bitmap_table_offset       0x500000
@@ -50,6 +59,15 @@ type                      1
 granularity_bits          16
 name_size                 2
 extra_data_size           0
+Bitmap table   type            offset                   size
+0              serialized      5177344                  65536
+1              all-zeroes      0                        65536
+2              all-zeroes      0                        65536
+3              all-zeroes      0                        65536
+4              all-zeroes      0                        65536
+5              all-zeroes      0                        65536
+6              all-zeroes      0                        65536
+7              all-zeroes      0                        65536
 
 
 === Bitmap preservation not possible to non-qcow2 ===
@@ -124,6 +142,15 @@ type                      1
 granularity_bits          19
 name_size                 2
 extra_data_size           0
+Bitmap table   type            offset                   size
+0              serialized      4587520                  65536
+1              all-zeroes      0                        65536
+2              all-zeroes      0                        65536
+3              all-zeroes      0                        65536
+4              all-zeroes      0                        65536
+5              all-zeroes      0                        65536
+6              all-zeroes      0                        65536
+7              all-zeroes      0                        65536
 
 Bitmap name               b2
 bitmap_table_offset       0x490000
@@ -133,6 +160,15 @@ type                      1
 granularity_bits          16
 name_size                 2
 extra_data_size           0
+Bitmap table   type            offset                   size
+0              serialized      4718592                  65536
+1              serialized      4294967296               65536
+2              serialized      5348033147437056         65536
+3              serialized      13792273858822144        65536
+4              serialized      4718592                  65536
+5              serialized      4294967296               65536
+6              serialized      4503608217305088         65536
+7              serialized      14073748835532800        65536
 
 Bitmap name               b0
 bitmap_table_offset       0x510000
@@ -142,6 +178,15 @@ type                      1
 granularity_bits          16
 name_size                 2
 extra_data_size           0
+Bitmap table   type            offset                   size
+0              serialized      5242880                  65536
+1              all-zeroes      0                        65536
+2              all-zeroes      0                        65536
+3              all-zeroes      0                        65536
+4              all-zeroes      0                        65536
+5              all-zeroes      0                        65536
+6              all-zeroes      0                        65536
+7              all-zeroes      0                        65536
 
 
 === Check bitmap contents ===
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index cfd9646..b512f84 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -171,14 +171,56 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
         entry_raw_size = self.bitmap_dir_entry_raw_size()
         padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
         fd.seek(padding, 1)
+        position = fd.tell()
+        self.read_bitmap_table(fd)
+        fd.seek(position)
 
     def bitmap_dir_entry_raw_size(self):
         return struct.calcsize(self.fmt) + self.name_size + \
             self.extra_data_size
 
+    def read_bitmap_table(self, fd):
+        fd.seek(self.bitmap_table_offset)
+        table_size = self.bitmap_table_size * 8 * 8
+        table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))]
+        self.bitmap_table = Qcow2BitmapTable(raw_table=table,
+                                             cluster_size=self.cluster_size)
+
     def dump(self):
         print(f'{"Bitmap name":<25} {self.name}')
         super(Qcow2BitmapDirEntry, self).dump()
+        self.bitmap_table.dump()
+
+
+class Qcow2BitmapTableEntry:
+
+    BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffffffffffe00
+    BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
+
+    def __init__(self, entry):
+        self.offset = entry & self.BME_TABLE_ENTRY_OFFSET_MASK
+        if self.offset:
+            self.type = 'serialized'
+        elif entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+            self.type = 'all-ones'
+        else:
+            self.type = 'all-zeroes'
+
+
+class Qcow2BitmapTable:
+
+    def __init__(self, raw_table, cluster_size):
+        self.entries = []
+        self.cluster_size = cluster_size
+        for entry in raw_table:
+            self.entries.append(Qcow2BitmapTableEntry(entry))
+
+    def dump(self):
+        size = self.cluster_size
+        bitmap_table = enumerate(self.entries)
+        print(f'{"Bitmap table":<14} {"type":<15} {"offset":<24} {"size"}')
+        for i, entry in bitmap_table:
+            print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
 
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
-- 
1.8.3.1

[PATCH v8 08/10] qcow2.py: Introduce '-j' key to dump in JSON format

[Andrey Shinkevich]

Add the command key to the qcow2.py arguments list to dump QCOW2
metadata in JSON format. Here is the suggested way to do that. The
implementation of the dump in JSON format is in the patch that follows.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py        | 19 +++++++++++++++----
 tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 0910e6a..7402279 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -26,16 +26,19 @@ from qcow2_format import (
 )
 
 
+dump_json = False
+
+
 def cmd_dump_header(fd):
     h = QcowHeader(fd)
-    h.dump()
+    h.dump(dump_json)
     print()
-    h.dump_extensions()
+    h.dump_extensions(dump_json)
 
 
 def cmd_dump_header_exts(fd):
     h = QcowHeader(fd)
-    h.dump_extensions()
+    h.dump_extensions(dump_json)
 
 
 def cmd_set_header(fd, name, value):
@@ -134,6 +137,11 @@ cmds = [
 
 
 def main(filename, cmd, args):
+    global dump_json
+    dump_json = '-j' in sys.argv
+    if dump_json:
+        sys.argv.remove('-j')
+        args.remove('-j')
     fd = open(filename, "r+b")
     try:
         for name, handler, num_args, desc in cmds:
@@ -151,11 +159,14 @@ def main(filename, cmd, args):
 
 
 def usage():
-    print("Usage: %s <file> <cmd> [<arg>, ...]" % sys.argv[0])
+    print("Usage: %s <file> <cmd> [<arg>, ...] [<key>, ...]" % sys.argv[0])
     print("")
     print("Supported commands:")
     for name, handler, num_args, desc in cmds:
         print("    %-20s - %s" % (name, desc))
+    print("")
+    print("Supported keys:")
+    print("    %-20s - %s" % ('-j', 'Dump in JSON format'))
 
 
 if __name__ == '__main__':
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index b512f84..dae6b6e 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -110,7 +110,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
         self.__dict__ = dict((field[2], values[i])
                              for i, field in enumerate(self.fields))
 
-    def dump(self):
+    def dump(self, dump_json=None):
         for f in self.fields:
             value = self.__dict__[f[2]]
             if isinstance(f[1], str):
@@ -141,8 +141,8 @@ class Qcow2BitmapExt(Qcow2Struct):
             [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
              for _ in range(self.nb_bitmaps)]
 
-    def dump(self):
-        super().dump()
+    def dump(self, dump_json=None):
+        super().dump(dump_json)
         for entry in self.bitmap_directory:
             print()
             entry.dump()
@@ -186,7 +186,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
         self.bitmap_table = Qcow2BitmapTable(raw_table=table,
                                              cluster_size=self.cluster_size)
 
-    def dump(self):
+    def dump(self, dump_json=None):
         print(f'{"Bitmap name":<25} {self.name}')
         super(Qcow2BitmapDirEntry, self).dump()
         self.bitmap_table.dump()
@@ -294,13 +294,13 @@ class QcowHeaderExtension(Qcow2Struct):
         else:
             self.obj = None
 
-    def dump(self):
+    def dump(self, dump_json=None):
         super().dump()
 
         if self.obj is None:
             print(f'{"data":<25} {self.data_str}')
         else:
-            self.obj.dump()
+            self.obj.dump(dump_json)
 
     @classmethod
     def create(cls, magic, data):
@@ -399,8 +399,8 @@ class QcowHeader(Qcow2Struct):
         buf = buf[0:header_bytes-1]
         fd.write(buf)
 
-    def dump_extensions(self):
+    def dump_extensions(self, dump_json=None):
         for ex in self.extensions:
             print('Header extension:')
-            ex.dump()
+            ex.dump(dump_json)
             print()
-- 
1.8.3.1

[PATCH v8 09/10] qcow2_format.py: collect fields to dump in JSON format

[Andrey Shinkevich]

As __dict__ is being extended with class members we do not want to
print, make a light copy of the initial __dict__ and extend the copy
by adding lists we have to print in the JSON output.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index dae6b6e..e8eb36f 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -110,6 +110,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
         self.__dict__ = dict((field[2], values[i])
                              for i, field in enumerate(self.fields))
 
+        self.fields_dict = self.__dict__.copy()
+
     def dump(self, dump_json=None):
         for f in self.fields:
             value = self.__dict__[f[2]]
@@ -140,6 +142,7 @@ class Qcow2BitmapExt(Qcow2Struct):
         self.bitmap_directory = \
             [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
              for _ in range(self.nb_bitmaps)]
+        self.fields_dict.update(bitmap_directory=self.bitmap_directory)
 
     def dump(self, dump_json=None):
         super().dump(dump_json)
@@ -185,6 +188,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
         table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))]
         self.bitmap_table = Qcow2BitmapTable(raw_table=table,
                                              cluster_size=self.cluster_size)
+        self.fields_dict.update(bitmap_table=self.bitmap_table)
 
     def dump(self, dump_json=None):
         print(f'{"Bitmap name":<25} {self.name}')
-- 
1.8.3.1

[PATCH v8 10/10] qcow2_format.py: support dumping metadata in JSON format

[Andrey Shinkevich]

Implementation of dumping QCOW2 image metadata.
The sample output:
{
    "Header_extensions": [
        {
            "name": "Feature table",
            "magic": 1745090647,
            "length": 192,
            "data_str": "<binary>"
        },
        {
            "name": "Bitmaps",
            "magic": 595929205,
            "length": 24,
            "data": {
                "nb_bitmaps": 2,
                "reserved32": 0,
                "bitmap_directory_size": 64,
                "bitmap_directory_offset": 1048576,
                "bitmap_directory": [
                    {
                        "name": "bitmap-1",
                        "bitmap_table_offset": 589824,
                        "bitmap_table_size": 1,
                        "flags": 2,
                        "type": 1,
                        "granularity_bits": 15,
                        "name_size": 8,
                        "extra_data_size": 0,
                        "bitmap_table": {
                            "entries": [
                                {
                                    "type": "serialized",
                                    "offset": 655360
                                },
                                ...

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 59 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index e8eb36f..70be4a1 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -19,6 +19,15 @@
 
 import struct
 import string
+import json
+
+
+class ComplexEncoder(json.JSONEncoder):
+    def default(self, obj):
+        if hasattr(obj, 'get_fields_dict'):
+            return obj.get_fields_dict()
+        else:
+            return json.JSONEncoder.default(self, obj)
 
 
 class Qcow2Field:
@@ -113,6 +122,11 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
         self.fields_dict = self.__dict__.copy()
 
     def dump(self, dump_json=None):
+        if dump_json:
+            print(json.dumps(self.get_fields_dict(), indent=4,
+                             cls=ComplexEncoder))
+            return
+
         for f in self.fields:
             value = self.__dict__[f[2]]
             if isinstance(f[1], str):
@@ -150,6 +164,9 @@ class Qcow2BitmapExt(Qcow2Struct):
             print()
             entry.dump()
 
+    def get_fields_dict(self):
+        return self.fields_dict
+
 
 class Qcow2BitmapDirEntry(Qcow2Struct):
 
@@ -195,6 +212,11 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
         super(Qcow2BitmapDirEntry, self).dump()
         self.bitmap_table.dump()
 
+    def get_fields_dict(self):
+        bmp_name = dict(name=self.name)
+        bme_dict = {**bmp_name, **self.fields_dict}
+        return bme_dict
+
 
 class Qcow2BitmapTableEntry:
 
@@ -210,6 +232,9 @@ class Qcow2BitmapTableEntry:
         else:
             self.type = 'all-zeroes'
 
+    def get_fields_dict(self):
+        return dict(type=self.type, offset=self.offset)
+
 
 class Qcow2BitmapTable:
 
@@ -226,6 +251,18 @@ class Qcow2BitmapTable:
         for i, entry in bitmap_table:
             print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
 
+    def get_fields_dict(self):
+        return dict(entries=self.entries)
+
+
+class Qcow2HeaderExtensionsDoc:
+
+    def __init__(self, extensions):
+        self.extensions = extensions
+
+    def get_fields_dict(self):
+        return dict(Header_extensions=self.extensions)
+
 
 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
 
@@ -241,6 +278,9 @@ class QcowHeaderExtension(Qcow2Struct):
             0x44415441: 'Data file'
         }
 
+        def get_fields_dict(self):
+            return self.mapping.get(self.value, "<unknown>")
+
     fields = (
         ('u32', Magic, 'magic'),
         ('u32', '{}', 'length')
@@ -306,6 +346,16 @@ class QcowHeaderExtension(Qcow2Struct):
         else:
             self.obj.dump(dump_json)
 
+    def get_fields_dict(self):
+        ext_name = dict(name=self.Magic(self.magic))
+        he_dict = {**ext_name, **self.fields_dict}
+        if self.obj is not None:
+            he_dict.update(data=self.obj)
+        else:
+            he_dict.update(data_str=self.data_str)
+
+        return he_dict
+
     @classmethod
     def create(cls, magic, data):
         return QcowHeaderExtension(magic, len(data), data)
@@ -404,7 +454,16 @@ class QcowHeader(Qcow2Struct):
         fd.write(buf)
 
     def dump_extensions(self, dump_json=None):
+        if dump_json:
+            ext_doc = Qcow2HeaderExtensionsDoc(self.extensions)
+            print(json.dumps(ext_doc.get_fields_dict(), indent=4,
+                             cls=ComplexEncoder))
+            return
+
         for ex in self.extensions:
             print('Header extension:')
             ex.dump(dump_json)
             print()
+
+    def get_fields_dict(self):
+        return self.fields_dict
-- 
1.8.3.1

Re: [PATCH v8 07/10] qcow2_format.py: Dump bitmap table serialized entries

[Andrey Shinkevich]

On 03.07.2020 16:13, Andrey Shinkevich wrote:
> Add bitmap table information to the QCOW2 metadata dump.
...
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -171,14 +171,56 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>           entry_raw_size = self.bitmap_dir_entry_raw_size()
>           padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
>           fd.seek(padding, 1)
> +        position = fd.tell()
> +        self.read_bitmap_table(fd)
> +        fd.seek(position)
>   
>       def bitmap_dir_entry_raw_size(self):
>           return struct.calcsize(self.fmt) + self.name_size + \
>               self.extra_data_size
>   
> +    def read_bitmap_table(self, fd):
> +        fd.seek(self.bitmap_table_offset)
> +        table_size = self.bitmap_table_size * 8 * 8
> +        table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))]
> +        self.bitmap_table = Qcow2BitmapTable(raw_table=table,
> +                                             cluster_size=self.cluster_size)

As an option, one make the class Qcow2BitmapTableEntry derived from 
Qcow2Struct

with the member:

fields = (

('u64', '{:#x}', 'table_entry')

)

and read entry by entry bitmap_table_size times from the disk in a loop.


Andrey

> +
>       def dump(self):
>           print(f'{"Bitmap name":<25} {self.name}')
>           super(Qcow2BitmapDirEntry, self).dump()
> +        self.bitmap_table.dump()
> +
> +
> +class Qcow2BitmapTableEntry:
> +
> +    BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffffffffffe00
> +    BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
> +
> +    def __init__(self, entry):
> +        self.offset = entry & self.BME_TABLE_ENTRY_OFFSET_MASK
> +        if self.offset:
> +            self.type = 'serialized'
> +        elif entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
> +            self.type = 'all-ones'
> +        else:
> +            self.type = 'all-zeroes'
> +
> +
> +class Qcow2BitmapTable:
> +
> +    def __init__(self, raw_table, cluster_size):
> +        self.entries = []
> +        self.cluster_size = cluster_size
> +        for entry in raw_table:
> +            self.entries.append(Qcow2BitmapTableEntry(entry))
> +
> +    def dump(self):
> +        size = self.cluster_size
> +        bitmap_table = enumerate(self.entries)
> +        print(f'{"Bitmap table":<14} {"type":<15} {"offset":<24} {"size"}')
> +        for i, entry in bitmap_table:
> +            print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
>   
>   
>   QCOW2_EXT_MAGIC_BITMAPS = 0x23852875

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

Re: [PATCH v8 04/10] qcow2_format.py: dump bitmap flags in human readable way.

[Vladimir Sementsov-Ogievskiy]

03.07.2020 16:13, Andrey Shinkevich wrote:
> Introduce the class BitmapFlags that parses a bitmap flags mask.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2_format.py | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index 1435e34..d8c058d 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -40,6 +40,22 @@ class Flags64(Qcow2Field):
>           return str(bits)
>   
>   
> +class BitmapFlags(Qcow2Field):
> +
> +    flags = {
> +        0x1: 'in-use',
> +        0x2: 'auto'
> +    }
> +
> +    def __str__(self):
> +        bits = []
> +        for bit in range(64):
> +            flag = self.value & (1 << bit)
> +            if flag:
> +                bits.append(self.flags.get(flag, '{:#x}'.format(flag)))

please use f-strings where possible, for consistency.

Also, more obvious notation for unknown bits would be f'bit-{bit}' I think, so you see number of bit, not big hex number.

> +        return f'{self.value:#x} ({bits})'
> +
> +
>   class Enum(Qcow2Field):
>   
>       def __str__(self):
> 


-- 
Best regards,
Vladimir

Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Vladimir Sementsov-Ogievskiy]

03.07.2020 16:13, Andrey Shinkevich wrote:
> Read and dump entries from the bitmap directory of QCOW2 image.
> It extends the output in the test case #291.
> 
> Header extension:
> magic                     0x23852875 (Bitmaps)
> ...
> 
> Bitmap name               bitmap-1
> bitmap_table_offset       0xf0000
> bitmap_table_size         1
> flags                     0x2 (['auto'])
> type                      1
> granularity_bits          16
> name_size                 8
> extra_data_size           0
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/291.out         | 45 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/qcow2_format.py | 44 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 89 insertions(+)
> 
> diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
> index 08bfaaa..53a8eeb 100644
> --- a/tests/qemu-iotests/291.out
> +++ b/tests/qemu-iotests/291.out
> @@ -33,6 +33,24 @@ reserved32                0
>   bitmap_directory_size     0x40
>   bitmap_directory_offset   0x510000
>   
> +Bitmap name               b1
> +bitmap_table_offset       0x4e0000
> +bitmap_table_size         1
> +flags                     0x0 ([])
> +type                      1
> +granularity_bits          19
> +name_size                 2
> +extra_data_size           0
> +
> +Bitmap name               b2
> +bitmap_table_offset       0x500000
> +bitmap_table_size         1
> +flags                     0x2 (['auto'])
> +type                      1
> +granularity_bits          16
> +name_size                 2
> +extra_data_size           0
> +
>   
>   === Bitmap preservation not possible to non-qcow2 ===
>   
> @@ -98,6 +116,33 @@ reserved32                0
>   bitmap_directory_size     0x60
>   bitmap_directory_offset   0x520000
>   
> +Bitmap name               b1
> +bitmap_table_offset       0x470000
> +bitmap_table_size         1
> +flags                     0x0 ([])
> +type                      1
> +granularity_bits          19
> +name_size                 2
> +extra_data_size           0
> +
> +Bitmap name               b2
> +bitmap_table_offset       0x490000
> +bitmap_table_size         1
> +flags                     0x2 (['auto'])
> +type                      1
> +granularity_bits          16
> +name_size                 2
> +extra_data_size           0
> +
> +Bitmap name               b0
> +bitmap_table_offset       0x510000
> +bitmap_table_size         1
> +flags                     0x0 ([])
> +type                      1
> +granularity_bits          16
> +name_size                 2
> +extra_data_size           0
> +
>   
>   === Check bitmap contents ===
>   
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index d8c058d..7c0dc9a 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
>   
>       def __init__(self, fd):
>           super().__init__(fd=fd)
> +        self.read_bitmap_directory(fd)
> +
> +    def read_bitmap_directory(self, fd):
> +        fd.seek(self.bitmap_directory_offset)
> +        self.bitmap_directory = \
> +            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]

sounds good. I think, we should restore fd position after reading bitmap_directory, to point at the end of extension, to not break further extensions loading

> +
> +    def dump(self):
> +        super().dump()
> +        for entry in self.bitmap_directory:
> +            print()
> +            entry.dump()
> +
> +
> +class Qcow2BitmapDirEntry(Qcow2Struct):
> +
> +    fields = (
> +        ('u64', '{:#x}', 'bitmap_table_offset'),
> +        ('u32', '{}', 'bitmap_table_size'),
> +        ('u32', BitmapFlags, 'flags'),
> +        ('u8',  '{}', 'type'),
> +        ('u8',  '{}', 'granularity_bits'),
> +        ('u16', '{}', 'name_size'),
> +        ('u32', '{}', 'extra_data_size')
> +    )
> +
> +    def __init__(self, fd):
> +        super().__init__(fd=fd)
> +        # Seek relative to the current position in the file
> +        fd.seek(self.extra_data_size, 1)
> +        bitmap_name = fd.read(self.name_size)
> +        self.name = bitmap_name.decode('ascii')
> +        # Move position to the end of the entry in the directory
> +        entry_raw_size = self.bitmap_dir_entry_raw_size()
> +        padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
> +        fd.seek(padding, 1)
> +
> +    def bitmap_dir_entry_raw_size(self):
> +        return struct.calcsize(self.fmt) + self.name_size + \
> +            self.extra_data_size
> +
> +    def dump(self):
> +        print(f'{"Bitmap name":<25} {self.name}')
> +        super(Qcow2BitmapDirEntry, self).dump()
>   
>   
>   QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
> 


-- 
Best regards,
Vladimir

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

[Andrey Shinkevich]

On 11.07.2020 19:34, Vladimir Sementsov-Ogievskiy wrote:
> 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?


To conform to the common style for comments in the file as it is at

class QcowHeaderExtension::__init__()


>
>>         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


For Qcow2BitmapExt, we need both fd and data and they are mutualy exclusive

in the constructor of the class Qcow2Struct. Rereading the bitmap extension

is a solution without changing the Qcow2Struct. Any other suggestion?

Andrey


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

Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Andrey Shinkevich]

On 11.07.2020 22:11, Vladimir Sementsov-Ogievskiy wrote:
> 03.07.2020 16:13, Andrey Shinkevich wrote:
>> Read and dump entries from the bitmap directory of QCOW2 image.
>> It extends the output in the test case #291.
>>
>>
...
>>   diff --git a/tests/qemu-iotests/qcow2_format.py 
>> b/tests/qemu-iotests/qcow2_format.py
>> index d8c058d..7c0dc9a 100644
>> --- a/tests/qemu-iotests/qcow2_format.py
>> +++ b/tests/qemu-iotests/qcow2_format.py
>> @@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
>>         def __init__(self, fd):
>>           super().__init__(fd=fd)
>> +        self.read_bitmap_directory(fd)
>> +
>> +    def read_bitmap_directory(self, fd):
>> +        fd.seek(self.bitmap_directory_offset)
>> +        self.bitmap_directory = \
>> +            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
>
> sounds good. I think, we should restore fd position after reading 
> bitmap_directory, to point at the end of extension, to not break 
> further extensions loading
>
>
Yes, it is done in the constructor of QcowHeaderExtension:

if self.magic == QCOW2_EXT_MAGIC_BITMAPS:

...

position = fd.tell()

...

self.obj = Qcow2BitmapExt(fd=fd)

fd.seek(position)


Andrey

Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Vladimir Sementsov-Ogievskiy]

13.07.2020 10:07, Andrey Shinkevich wrote:
> On 11.07.2020 22:11, Vladimir Sementsov-Ogievskiy wrote:
>> 03.07.2020 16:13, Andrey Shinkevich wrote:
>>> Read and dump entries from the bitmap directory of QCOW2 image.
>>> It extends the output in the test case #291.
>>>
>>>
> ...
>>>   diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
>>> index d8c058d..7c0dc9a 100644
>>> --- a/tests/qemu-iotests/qcow2_format.py
>>> +++ b/tests/qemu-iotests/qcow2_format.py
>>> @@ -132,6 +132,50 @@ class Qcow2BitmapExt(Qcow2Struct):
>>>         def __init__(self, fd):
>>>           super().__init__(fd=fd)
>>> +        self.read_bitmap_directory(fd)
>>> +
>>> +    def read_bitmap_directory(self, fd):
>>> +        fd.seek(self.bitmap_directory_offset)
>>> +        self.bitmap_directory = \
>>> +            [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
>>
>> sounds good. I think, we should restore fd position after reading bitmap_directory, to point at the end of extension, to not break further extensions loading
>>
>>
> Yes, it is done in the constructor of QcowHeaderExtension:
> 
> if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
> 
> ...
> 
> position = fd.tell()
> 
> ...
> 
> self.obj = Qcow2BitmapExt(fd=fd)
> 
> fd.seek(position)
> 

I don't like it. If you want caller to care about fd, caller should know size of created child. But passing fd to constructor implies that caller not aware of size of new created structure. So I think good api is: constuctor starts to read the structure and left after this structure on exit from consturctor (so, caller may read following structures). Constructor may read some nested structures, but is responsible for restoring fd after it.


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

13.07.2020 07:49, Andrey Shinkevich wrote:
> On 11.07.2020 19:34, Vladimir Sementsov-Ogievskiy wrote:
>> 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?
> 
> 
> To conform to the common style for comments in the file as it is at
> 
> class QcowHeaderExtension::__init__()

It's OK to additionally fix/change style in the chunk where you change some logic anyway. But changing style in another places - I think it doesn't worth it. Remember that it may make patch porting more complicated for nothing.

And this is not only my position, https://wiki.qemu.org/Contribute/SubmitAPatch directly statates it in "Don't include irrelevant changes" paragraph:

     In particular, don't include formatting, coding style or whitespace changes to bits of code that would otherwise not be touched by the patch.

> 
> 
>>
>>>         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
> 
> 
> For Qcow2BitmapExt, we need both fd and data and they are mutualy exclusive
> 
> in the constructor of the class Qcow2Struct. Rereading the bitmap extension
> 
> is a solution without changing the Qcow2Struct. Any other suggestion?
> 

I think, we want to modify QcowHeaderExtension so that it creates/reads its children appropriately. Probably, for bitmaps extension exclusively we pass fd to Qcow2BitmapExt, not reading data by ourselves, and for other extensions keep current semantics of just reading data by hand (or you may add a simple class Qcow2Ext.

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


-- 
Best regards,
Vladimir

Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Vladimir Sementsov-Ogievskiy]

03.07.2020 16:13, Andrey Shinkevich wrote:
> Read and dump entries from the bitmap directory of QCOW2 image.
> It extends the output in the test case #291.
> 
> Header extension:
> magic                     0x23852875 (Bitmaps)
> ...
> 
> Bitmap name               bitmap-1
> bitmap_table_offset       0xf0000
> bitmap_table_size         1
> flags                     0x2 (['auto'])
> type                      1
> granularity_bits          16
> name_size                 8
> extra_data_size           0
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v8 05/10] qcow2_format.py: Dump bitmap directory information

[Vladimir Sementsov-Ogievskiy]

16.07.2020 12:13, Vladimir Sementsov-Ogievskiy wrote:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Oops, sorry, I wanted to answer v10 patch. Ignore this.


-- 
Best regards,
Vladimir