[PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata

[PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is my suggestion to substitute only first three patches :) of
Andrey's [PATCH v3 0/6] iotests: Dump QCOW2 dirty bitmaps metadata

so, I called it v4 for convenience.

What is here:
1. First, update code style
2. Next, try to refactor in a manner which will make adding new data
structures simple (look at Qcow2BitmapExt class in last patch)

I think, next step is to add type hints. Then add more structures.
And, anyway, at some point we should move it into python/ directory (at
least qcow2_format.py lib)

Vladimir Sementsov-Ogievskiy (12):
  qcow2.py: python style fixes
  qcow2.py: move qcow2 format classes to separate module
  qcow2_format.py: drop new line printing at end of dump()
  qcow2_format.py: use tuples instead of lists for fields
  qcow2_format.py: use modern string formatting
  qcow2_format.py: use strings to specify c-type of struct fields
  qcow2_format.py: separate generic functionality of structure classes
  qcow2_format.py: add field-formatting class
  qcow2_format.py: QcowHeaderExtension: add dump method
  qcow2_format: refactor QcowHeaderExtension as a subclass of
    Qcow2Struct
  qcow2: QcowHeaderExtension print names for extension magics
  qcow2_format.py: dump bitmaps header extension

 tests/qemu-iotests/031.out         |  22 +--
 tests/qemu-iotests/036.out         |   4 +-
 tests/qemu-iotests/061.out         |  14 +-
 tests/qemu-iotests/291             |   4 +
 tests/qemu-iotests/291.out         |  33 ++++
 tests/qemu-iotests/qcow2.py        | 200 ++++------------------
 tests/qemu-iotests/qcow2_format.py | 260 +++++++++++++++++++++++++++++
 7 files changed, 348 insertions(+), 189 deletions(-)
 create mode 100644 tests/qemu-iotests/qcow2_format.py

-- 
2.21.0

[PATCH v4 01/12] qcow2.py: python style fixes

[Vladimir Sementsov-Ogievskiy]

Fix flake8 complains. Leave the only chunk of lines over 79 characters:
initialization of cmds variable. Leave it for another day, when it
should be refactored to utilize argparse instead of hand-written
parsing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py | 92 +++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 39 deletions(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 94a07b2f6f..539f5a186b 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -4,6 +4,7 @@ import sys
 import struct
 import string
 
+
 class QcowHeaderExtension:
 
     def __init__(self, magic, length, data):
@@ -11,14 +12,15 @@ class QcowHeaderExtension:
             padding = 8 - (length % 8)
             data += b"\0" * padding
 
-        self.magic  = magic
+        self.magic = magic
         self.length = length
-        self.data   = data
+        self.data = data
 
     @classmethod
     def create(cls, magic, data):
         return QcowHeaderExtension(magic, len(data), data)
 
+
 class QcowHeader:
 
     uint32_t = 'I'
@@ -26,27 +28,27 @@ class QcowHeader:
 
     fields = [
         # Version 2 header fields
-        [ uint32_t, '%#x',  'magic' ],
-        [ uint32_t, '%d',   'version' ],
-        [ uint64_t, '%#x',  'backing_file_offset' ],
-        [ uint32_t, '%#x',  'backing_file_size' ],
-        [ uint32_t, '%d',   'cluster_bits' ],
-        [ uint64_t, '%d',   'size' ],
-        [ uint32_t, '%d',   'crypt_method' ],
-        [ uint32_t, '%d',   'l1_size' ],
-        [ uint64_t, '%#x',  'l1_table_offset' ],
-        [ uint64_t, '%#x',  'refcount_table_offset' ],
-        [ uint32_t, '%d',   'refcount_table_clusters' ],
-        [ uint32_t, '%d',   'nb_snapshots' ],
-        [ uint64_t, '%#x',  'snapshot_offset' ],
+        [uint32_t, '%#x',  'magic'],
+        [uint32_t, '%d',   'version'],
+        [uint64_t, '%#x',  'backing_file_offset'],
+        [uint32_t, '%#x',  'backing_file_size'],
+        [uint32_t, '%d',   'cluster_bits'],
+        [uint64_t, '%d',   'size'],
+        [uint32_t, '%d',   'crypt_method'],
+        [uint32_t, '%d',   'l1_size'],
+        [uint64_t, '%#x',  'l1_table_offset'],
+        [uint64_t, '%#x',  'refcount_table_offset'],
+        [uint32_t, '%d',   'refcount_table_clusters'],
+        [uint32_t, '%d',   'nb_snapshots'],
+        [uint64_t, '%#x',  'snapshot_offset'],
 
         # Version 3 header fields
-        [ uint64_t, 'mask', 'incompatible_features' ],
-        [ uint64_t, 'mask', 'compatible_features' ],
-        [ uint64_t, 'mask', 'autoclear_features' ],
-        [ uint32_t, '%d',   'refcount_order' ],
-        [ uint32_t, '%d',   'header_length' ],
-    ];
+        [uint64_t, 'mask', 'incompatible_features'],
+        [uint64_t, 'mask', 'compatible_features'],
+        [uint64_t, 'mask', 'autoclear_features'],
+        [uint32_t, '%d',   'refcount_order'],
+        [uint32_t, '%d',   'header_length'],
+    ]
 
     fmt = '>' + ''.join(field[0] for field in fields)
 
@@ -59,7 +61,7 @@ class QcowHeader:
 
         header = struct.unpack(QcowHeader.fmt, buf)
         self.__dict__ = dict((field[2], header[i])
-            for i, field in enumerate(QcowHeader.fields))
+                             for i, field in enumerate(QcowHeader.fields))
 
         self.set_defaults()
         self.cluster_size = 1 << self.cluster_bits
@@ -96,7 +98,8 @@ class QcowHeader:
             else:
                 padded = (length + 7) & ~7
                 data = fd.read(padded)
-                self.extensions.append(QcowHeaderExtension(magic, length, data))
+                self.extensions.append(QcowHeaderExtension(magic, length,
+                                                           data))
 
     def update_extensions(self, fd):
 
@@ -108,14 +111,13 @@ class QcowHeader:
             fd.write(buf)
             fd.write(ex.data)
 
-        if self.backing_file != None:
+        if self.backing_file is not None:
             self.backing_file_offset = fd.tell()
             fd.write(self.backing_file)
 
         if fd.tell() > self.cluster_size:
             raise Exception("I think I just broke the image...")
 
-
     def update(self, fd):
         header_bytes = self.header_length
 
@@ -163,19 +165,21 @@ def cmd_dump_header(fd):
     h.dump()
     h.dump_extensions()
 
+
 def cmd_dump_header_exts(fd):
     h = QcowHeader(fd)
     h.dump_extensions()
 
+
 def cmd_set_header(fd, name, value):
     try:
         value = int(value, 0)
-    except:
+    except ValueError:
         print("'%s' is not a valid number" % value)
         sys.exit(1)
 
     fields = (field[2] for field in QcowHeader.fields)
-    if not name in fields:
+    if name not in fields:
         print("'%s' is not a known header field" % name)
         sys.exit(1)
 
@@ -183,25 +187,29 @@ def cmd_set_header(fd, name, value):
     h.__dict__[name] = value
     h.update(fd)
 
+
 def cmd_add_header_ext(fd, magic, data):
     try:
         magic = int(magic, 0)
-    except:
+    except ValueError:
         print("'%s' is not a valid magic number" % magic)
         sys.exit(1)
 
     h = QcowHeader(fd)
-    h.extensions.append(QcowHeaderExtension.create(magic, data.encode('ascii')))
+    h.extensions.append(QcowHeaderExtension.create(magic,
+                                                   data.encode('ascii')))
     h.update(fd)
 
+
 def cmd_add_header_ext_stdio(fd, magic):
     data = sys.stdin.read()
     cmd_add_header_ext(fd, magic, data)
 
+
 def cmd_del_header_ext(fd, magic):
     try:
         magic = int(magic, 0)
-    except:
+    except ValueError:
         print("'%s' is not a valid magic number" % magic)
         sys.exit(1)
 
@@ -219,12 +227,13 @@ def cmd_del_header_ext(fd, magic):
 
     h.update(fd)
 
+
 def cmd_set_feature_bit(fd, group, bit):
     try:
         bit = int(bit, 0)
         if bit < 0 or bit >= 64:
             raise ValueError
-    except:
+    except ValueError:
         print("'%s' is not a valid bit number in range [0, 64)" % bit)
         sys.exit(1)
 
@@ -236,21 +245,24 @@ def cmd_set_feature_bit(fd, group, bit):
     elif group == 'autoclear':
         h.autoclear_features |= 1 << bit
     else:
-        print("'%s' is not a valid group, try 'incompatible', 'compatible', or 'autoclear'" % group)
+        print("'%s' is not a valid group, try "
+              "'incompatible', 'compatible', or 'autoclear'" % group)
         sys.exit(1)
 
     h.update(fd)
 
+
 cmds = [
-    [ 'dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions' ],
-    [ 'dump-header-exts',     cmd_dump_header_exts,     0, 'Dump image header extensions' ],
-    [ 'set-header',           cmd_set_header,           2, 'Set a field in the header'],
-    [ 'add-header-ext',       cmd_add_header_ext,       2, 'Add a header extension' ],
-    [ 'add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header extension, data from stdin' ],
-    [ 'del-header-ext',       cmd_del_header_ext,       1, 'Delete a header extension' ],
-    [ 'set-feature-bit',      cmd_set_feature_bit,      2, 'Set a feature bit'],
+    ['dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions'],
+    ['dump-header-exts',     cmd_dump_header_exts,     0, 'Dump image header extensions'],
+    ['set-header',           cmd_set_header,           2, 'Set a field in the header'],
+    ['add-header-ext',       cmd_add_header_ext,       2, 'Add a header extension'],
+    ['add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header extension, data from stdin'],
+    ['del-header-ext',       cmd_del_header_ext,       1, 'Delete a header extension'],
+    ['set-feature-bit',      cmd_set_feature_bit,      2, 'Set a feature bit'],
 ]
 
+
 def main(filename, cmd, args):
     fd = open(filename, "r+b")
     try:
@@ -267,6 +279,7 @@ def main(filename, cmd, args):
     finally:
         fd.close()
 
+
 def usage():
     print("Usage: %s <file> <cmd> [<arg>, ...]" % sys.argv[0])
     print("")
@@ -274,6 +287,7 @@ def usage():
     for name, handler, num_args, desc in cmds:
         print("    %-20s - %s" % (name, desc))
 
+
 if __name__ == '__main__':
     if len(sys.argv) < 3:
         usage()
-- 
2.21.0

Re: [PATCH v4 01/12] qcow2.py: python style fixes

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 01/12] qcow2.py: python style fixes

Fix flake8 complains. Leave the only chunk of lines over 79 characters:
initialization of cmds variable. Leave it for another day, when it
should be refactored to utilize argparse instead of hand-written
parsing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py | 92 +++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 39 deletions(-)

[-- Attachment #2: Type: text/html, Size: 2009 bytes --]

Re: [PATCH v4 01/12] qcow2.py: python style fixes

[Eric Blake]

On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> Fix flake8 complains. Leave the only chunk of lines over 79 characters:

complaints

> initialization of cmds variable. Leave it for another day, when it
> should be refactored to utilize argparse instead of hand-written
> parsing.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2.py | 92 +++++++++++++++++++++----------------
>   1 file changed, 53 insertions(+), 39 deletions(-)
> 

>   cmds = [
> -    [ 'dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions' ],
> -    [ 'dump-header-exts',     cmd_dump_header_exts,     0, 'Dump image header extensions' ],
> -    [ 'set-header',           cmd_set_header,           2, 'Set a field in the header'],
> -    [ 'add-header-ext',       cmd_add_header_ext,       2, 'Add a header extension' ],
> -    [ 'add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header extension, data from stdin' ],
> -    [ 'del-header-ext',       cmd_del_header_ext,       1, 'Delete a header extension' ],
> -    [ 'set-feature-bit',      cmd_set_feature_bit,      2, 'Set a feature bit'],
> +    ['dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions'],

I know you mentioned argparse as a later refactoring, but is it worth 
reflowing the table in the meantime?

['dump-header', cmd_dump_header, 0,
  'Dump image header and header extensions'],
[...
  '...'],

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 01/12] qcow2.py: python style fixes

[Vladimir Sementsov-Ogievskiy]

05.06.2020 22:43, Eric Blake wrote:
> On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Fix flake8 complains. Leave the only chunk of lines over 79 characters:
> 
> complaints
> 
>> initialization of cmds variable. Leave it for another day, when it
>> should be refactored to utilize argparse instead of hand-written
>> parsing.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/qcow2.py | 92 +++++++++++++++++++++----------------
>>   1 file changed, 53 insertions(+), 39 deletions(-)
>>
> 
>>   cmds = [
>> -    [ 'dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions' ],
>> -    [ 'dump-header-exts',     cmd_dump_header_exts,     0, 'Dump image header extensions' ],
>> -    [ 'set-header',           cmd_set_header,           2, 'Set a field in the header'],
>> -    [ 'add-header-ext',       cmd_add_header_ext,       2, 'Add a header extension' ],
>> -    [ 'add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header extension, data from stdin' ],
>> -    [ 'del-header-ext',       cmd_del_header_ext,       1, 'Delete a header extension' ],
>> -    [ 'set-feature-bit',      cmd_set_feature_bit,      2, 'Set a feature bit'],
>> +    ['dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions'],
> 
> I know you mentioned argparse as a later refactoring, but is it worth reflowing the table in the meantime?
> 
> ['dump-header', cmd_dump_header, 0,
>   'Dump image header and header extensions'],
> [...
>   '...'],
> 

Yes, now I think it worth doing, patchew emails never makes series more beautiful.

-- 
Best regards,
Vladimir

[PATCH v4 02/12] qcow2.py: move qcow2 format classes to separate module

[Vladimir Sementsov-Ogievskiy]

We are going to enhance qcow2 format parsing by adding more structure
classes. Let's split format parsing from utility code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py        | 161 +----------------------------
 tests/qemu-iotests/qcow2_format.py | 157 ++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 157 deletions(-)
 create mode 100644 tests/qemu-iotests/qcow2_format.py

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 539f5a186b..d9c41668fd 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -1,163 +1,10 @@
 #!/usr/bin/env python3
-
 import sys
-import struct
-import string
-
-
-class QcowHeaderExtension:
-
-    def __init__(self, magic, length, data):
-        if length % 8 != 0:
-            padding = 8 - (length % 8)
-            data += b"\0" * padding
-
-        self.magic = magic
-        self.length = length
-        self.data = data
-
-    @classmethod
-    def create(cls, magic, data):
-        return QcowHeaderExtension(magic, len(data), data)
-
-
-class QcowHeader:
-
-    uint32_t = 'I'
-    uint64_t = 'Q'
-
-    fields = [
-        # Version 2 header fields
-        [uint32_t, '%#x',  'magic'],
-        [uint32_t, '%d',   'version'],
-        [uint64_t, '%#x',  'backing_file_offset'],
-        [uint32_t, '%#x',  'backing_file_size'],
-        [uint32_t, '%d',   'cluster_bits'],
-        [uint64_t, '%d',   'size'],
-        [uint32_t, '%d',   'crypt_method'],
-        [uint32_t, '%d',   'l1_size'],
-        [uint64_t, '%#x',  'l1_table_offset'],
-        [uint64_t, '%#x',  'refcount_table_offset'],
-        [uint32_t, '%d',   'refcount_table_clusters'],
-        [uint32_t, '%d',   'nb_snapshots'],
-        [uint64_t, '%#x',  'snapshot_offset'],
-
-        # Version 3 header fields
-        [uint64_t, 'mask', 'incompatible_features'],
-        [uint64_t, 'mask', 'compatible_features'],
-        [uint64_t, 'mask', 'autoclear_features'],
-        [uint32_t, '%d',   'refcount_order'],
-        [uint32_t, '%d',   'header_length'],
-    ]
-
-    fmt = '>' + ''.join(field[0] for field in fields)
-
-    def __init__(self, fd):
-
-        buf_size = struct.calcsize(QcowHeader.fmt)
-
-        fd.seek(0)
-        buf = fd.read(buf_size)
-
-        header = struct.unpack(QcowHeader.fmt, buf)
-        self.__dict__ = dict((field[2], header[i])
-                             for i, field in enumerate(QcowHeader.fields))
-
-        self.set_defaults()
-        self.cluster_size = 1 << self.cluster_bits
-
-        fd.seek(self.header_length)
-        self.load_extensions(fd)
-
-        if self.backing_file_offset:
-            fd.seek(self.backing_file_offset)
-            self.backing_file = fd.read(self.backing_file_size)
-        else:
-            self.backing_file = None
-
-    def set_defaults(self):
-        if self.version == 2:
-            self.incompatible_features = 0
-            self.compatible_features = 0
-            self.autoclear_features = 0
-            self.refcount_order = 4
-            self.header_length = 72
-
-    def load_extensions(self, fd):
-        self.extensions = []
-
-        if self.backing_file_offset != 0:
-            end = min(self.cluster_size, self.backing_file_offset)
-        else:
-            end = self.cluster_size
-
-        while fd.tell() < end:
-            (magic, length) = struct.unpack('>II', fd.read(8))
-            if magic == 0:
-                break
-            else:
-                padded = (length + 7) & ~7
-                data = fd.read(padded)
-                self.extensions.append(QcowHeaderExtension(magic, length,
-                                                           data))
-
-    def update_extensions(self, fd):
-
-        fd.seek(self.header_length)
-        extensions = self.extensions
-        extensions.append(QcowHeaderExtension(0, 0, b""))
-        for ex in extensions:
-            buf = struct.pack('>II', ex.magic, ex.length)
-            fd.write(buf)
-            fd.write(ex.data)
-
-        if self.backing_file is not None:
-            self.backing_file_offset = fd.tell()
-            fd.write(self.backing_file)
-
-        if fd.tell() > self.cluster_size:
-            raise Exception("I think I just broke the image...")
-
-    def update(self, fd):
-        header_bytes = self.header_length
-
-        self.update_extensions(fd)
-
-        fd.seek(0)
-        header = tuple(self.__dict__[f] for t, p, f in QcowHeader.fields)
-        buf = struct.pack(QcowHeader.fmt, *header)
-        buf = buf[0:header_bytes-1]
-        fd.write(buf)
-
-    def dump(self):
-        for f in QcowHeader.fields:
-            value = self.__dict__[f[2]]
-            if f[1] == 'mask':
-                bits = []
-                for bit in range(64):
-                    if value & (1 << bit):
-                        bits.append(bit)
-                value_str = str(bits)
-            else:
-                value_str = f[1] % value
-
-            print("%-25s" % f[2], value_str)
-        print("")
-
-    def dump_extensions(self):
-        for ex in self.extensions:
-
-            data = ex.data[:ex.length]
-            if all(c in string.printable.encode('ascii') for c in data):
-                data = "'%s'" % data.decode('ascii')
-            else:
-                data = "<binary>"
 
-            print("Header extension:")
-            print("%-25s %#x" % ("magic", ex.magic))
-            print("%-25s %d" % ("length", ex.length))
-            print("%-25s %s" % ("data", data))
-            print("")
+from qcow2_format import (
+    QcowHeader,
+    QcowHeaderExtension
+)
 
 
 def cmd_dump_header(fd):
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
new file mode 100644
index 0000000000..c7270a0a6e
--- /dev/null
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -0,0 +1,157 @@
+import struct
+import string
+
+
+class QcowHeaderExtension:
+
+    def __init__(self, magic, length, data):
+        if length % 8 != 0:
+            padding = 8 - (length % 8)
+            data += b"\0" * padding
+
+        self.magic = magic
+        self.length = length
+        self.data = data
+
+    @classmethod
+    def create(cls, magic, data):
+        return QcowHeaderExtension(magic, len(data), data)
+
+
+class QcowHeader:
+
+    uint32_t = 'I'
+    uint64_t = 'Q'
+
+    fields = [
+        # Version 2 header fields
+        [uint32_t, '%#x',  'magic'],
+        [uint32_t, '%d',   'version'],
+        [uint64_t, '%#x',  'backing_file_offset'],
+        [uint32_t, '%#x',  'backing_file_size'],
+        [uint32_t, '%d',   'cluster_bits'],
+        [uint64_t, '%d',   'size'],
+        [uint32_t, '%d',   'crypt_method'],
+        [uint32_t, '%d',   'l1_size'],
+        [uint64_t, '%#x',  'l1_table_offset'],
+        [uint64_t, '%#x',  'refcount_table_offset'],
+        [uint32_t, '%d',   'refcount_table_clusters'],
+        [uint32_t, '%d',   'nb_snapshots'],
+        [uint64_t, '%#x',  'snapshot_offset'],
+
+        # Version 3 header fields
+        [uint64_t, 'mask', 'incompatible_features'],
+        [uint64_t, 'mask', 'compatible_features'],
+        [uint64_t, 'mask', 'autoclear_features'],
+        [uint32_t, '%d',   'refcount_order'],
+        [uint32_t, '%d',   'header_length'],
+    ]
+
+    fmt = '>' + ''.join(field[0] for field in fields)
+
+    def __init__(self, fd):
+
+        buf_size = struct.calcsize(QcowHeader.fmt)
+
+        fd.seek(0)
+        buf = fd.read(buf_size)
+
+        header = struct.unpack(QcowHeader.fmt, buf)
+        self.__dict__ = dict((field[2], header[i])
+                             for i, field in enumerate(QcowHeader.fields))
+
+        self.set_defaults()
+        self.cluster_size = 1 << self.cluster_bits
+
+        fd.seek(self.header_length)
+        self.load_extensions(fd)
+
+        if self.backing_file_offset:
+            fd.seek(self.backing_file_offset)
+            self.backing_file = fd.read(self.backing_file_size)
+        else:
+            self.backing_file = None
+
+    def set_defaults(self):
+        if self.version == 2:
+            self.incompatible_features = 0
+            self.compatible_features = 0
+            self.autoclear_features = 0
+            self.refcount_order = 4
+            self.header_length = 72
+
+    def load_extensions(self, fd):
+        self.extensions = []
+
+        if self.backing_file_offset != 0:
+            end = min(self.cluster_size, self.backing_file_offset)
+        else:
+            end = self.cluster_size
+
+        while fd.tell() < end:
+            (magic, length) = struct.unpack('>II', fd.read(8))
+            if magic == 0:
+                break
+            else:
+                padded = (length + 7) & ~7
+                data = fd.read(padded)
+                self.extensions.append(QcowHeaderExtension(magic, length,
+                                                           data))
+
+    def update_extensions(self, fd):
+
+        fd.seek(self.header_length)
+        extensions = self.extensions
+        extensions.append(QcowHeaderExtension(0, 0, b""))
+        for ex in extensions:
+            buf = struct.pack('>II', ex.magic, ex.length)
+            fd.write(buf)
+            fd.write(ex.data)
+
+        if self.backing_file is not None:
+            self.backing_file_offset = fd.tell()
+            fd.write(self.backing_file)
+
+        if fd.tell() > self.cluster_size:
+            raise Exception("I think I just broke the image...")
+
+    def update(self, fd):
+        header_bytes = self.header_length
+
+        self.update_extensions(fd)
+
+        fd.seek(0)
+        header = tuple(self.__dict__[f] for t, p, f in QcowHeader.fields)
+        buf = struct.pack(QcowHeader.fmt, *header)
+        buf = buf[0:header_bytes-1]
+        fd.write(buf)
+
+    def dump(self):
+        for f in QcowHeader.fields:
+            value = self.__dict__[f[2]]
+            if f[1] == 'mask':
+                bits = []
+                for bit in range(64):
+                    if value & (1 << bit):
+                        bits.append(bit)
+                value_str = str(bits)
+            else:
+                value_str = f[1] % value
+
+            print("%-25s" % f[2], value_str)
+        print("")
+
+    def dump_extensions(self):
+        for ex in self.extensions:
+
+            data = ex.data[:ex.length]
+            if all(c in string.printable.encode('ascii') for c in data):
+                data = "'%s'" % data.decode('ascii')
+            else:
+                data = "<binary>"
+
+            print("Header extension:")
+            print("%-25s %#x" % ("magic", ex.magic))
+            print("%-25s %d" % ("length", ex.length))
+            print("%-25s %s" % ("data", data))
+            print("")
-- 
2.21.0

Re: [PATCH v4 02/12] qcow2.py: move qcow2 format classes to separate module

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 02/12] qcow2.py: move qcow2 format classes to separate module

We are going to enhance qcow2 format parsing by adding more structure
classes. Let's split format parsing from utility code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py        | 161 +----------------------------
 tests/qemu-iotests/qcow2_format.py | 157 ++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 157 deletions(-)
 create mode 100644 tests/qemu-iotests/qcow2_format.py

[-- Attachment #2: Type: text/html, Size: 2153 bytes --]

Re: [PATCH v4 02/12] qcow2.py: move qcow2 format classes to separate module

[Eric Blake]

On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to enhance qcow2 format parsing by adding more structure
> classes. Let's split format parsing from utility code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2.py        | 161 +----------------------------
>   tests/qemu-iotests/qcow2_format.py | 157 ++++++++++++++++++++++++++++
>   2 files changed, 161 insertions(+), 157 deletions(-)
>   create mode 100644 tests/qemu-iotests/qcow2_format.py
> 
> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
> index 539f5a186b..d9c41668fd 100755
> --- a/tests/qemu-iotests/qcow2.py
> +++ b/tests/qemu-iotests/qcow2.py
> @@ -1,163 +1,10 @@
>   #!/usr/bin/env python3
> -
>   import sys

Pre-existing: no copyright blurb on the old file...

> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -0,0 +1,157 @@
> +import struct
> +import string
> +

It would be nice to fix that, and have one on the new file as well.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 02/12] qcow2.py: move qcow2 format classes to separate module

[Vladimir Sementsov-Ogievskiy]

05.06.2020 23:14, Eric Blake wrote:
> On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
>> We are going to enhance qcow2 format parsing by adding more structure
>> classes. Let's split format parsing from utility code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/qcow2.py        | 161 +----------------------------
>>   tests/qemu-iotests/qcow2_format.py | 157 ++++++++++++++++++++++++++++
>>   2 files changed, 161 insertions(+), 157 deletions(-)
>>   create mode 100644 tests/qemu-iotests/qcow2_format.py
>>
>> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
>> index 539f5a186b..d9c41668fd 100755
>> --- a/tests/qemu-iotests/qcow2.py
>> +++ b/tests/qemu-iotests/qcow2.py
>> @@ -1,163 +1,10 @@
>>   #!/usr/bin/env python3
>> -
>>   import sys
> 
> Pre-existing: no copyright blurb on the old file...
> 
>> +++ b/tests/qemu-iotests/qcow2_format.py
>> @@ -0,0 +1,157 @@
>> +import struct
>> +import string
>> +
> 
> It would be nice to fix that, and have one on the new file as well.
> 


Yes, I thought about this.. But what to add? Seems OK to add Virtuozzo copyright and common "This program is free software; you can redist... GPL ..." header to qcow2_format.py, as we are going to rewrite and improve it a lot. But what to add to qcow2.py, to the part which is kept mostly unchanged?

-- 
Best regards,
Vladimir

Re: [PATCH v4 02/12] qcow2.py: move qcow2 format classes to separate module

[Eric Blake]

On 6/6/20 2:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 05.06.2020 23:14, Eric Blake wrote:
>> On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> We are going to enhance qcow2 format parsing by adding more structure
>>> classes. Let's split format parsing from utility code.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---

>>> +++ b/tests/qemu-iotests/qcow2.py
>>> @@ -1,163 +1,10 @@
>>>   #!/usr/bin/env python3
>>> -
>>>   import sys
>>
>> Pre-existing: no copyright blurb on the old file...
>>
>>> +++ b/tests/qemu-iotests/qcow2_format.py
>>> @@ -0,0 +1,157 @@
>>> +import struct
>>> +import string
>>> +
>>
>> It would be nice to fix that, and have one on the new file as well.
>>
> 
> 
> Yes, I thought about this.. But what to add? Seems OK to add Virtuozzo 
> copyright and common "This program is free software; you can redist... 
> GPL ..." header to qcow2_format.py, as we are going to rewrite and 
> improve it a lot. But what to add to qcow2.py, to the part which is kept 
> mostly unchanged?

Late answer, since I see you've already posted v5, but any file without 
an explicit header is implicitly GPLv2+ by virtue of the top-level 
LICENSE file.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v4 03/12] qcow2_format.py: drop new line printing at end of dump()

[Vladimir Sementsov-Ogievskiy]

This will simplify further conversion. To compensate, print this empty
line directly in cmd_dump_header().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py        | 1 +
 tests/qemu-iotests/qcow2_format.py | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index d9c41668fd..79db81b040 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -10,6 +10,7 @@ from qcow2_format import (
 def cmd_dump_header(fd):
     h = QcowHeader(fd)
     h.dump()
+    print()
     h.dump_extensions()
 
 
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index c7270a0a6e..99e5248e73 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -139,7 +139,6 @@ class QcowHeader:
                 value_str = f[1] % value
 
             print("%-25s" % f[2], value_str)
-        print("")
 
     def dump_extensions(self):
         for ex in self.extensions:
-- 
2.21.0

Re: [PATCH v4 03/12] qcow2_format.py: drop new line printing at end of dump()

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 03/12] qcow2_format.py: drop new line printing at end of dump()

This will simplify further conversion. To compensate, print this empty
line directly in cmd_dump_header().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py        | 1 +
 tests/qemu-iotests/qcow2_format.py | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index d9c41668fd..79db81b040 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -10,6 +10,7 @@ from qcow2_format import (
 def cmd_dump_header(fd):
     h = QcowHeader(fd)
     h.dump()
+    print()
     h.dump_extensions()


diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index c7270a0a6e..99e5248e73 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -139,7 +139,6 @@ class QcowHeader:
                 value_str = f[1] % value

             print("%-25s" % f[2], value_str)
-        print("")

     def dump_extensions(self):
         for ex in self.extensions:
--
2.21.0


[-- Attachment #2: Type: text/html, Size: 3185 bytes --]

[PATCH v4 04/12] qcow2_format.py: use tuples instead of lists for fields

[Vladimir Sementsov-Ogievskiy]

No need in lists: it's a constant variable.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 40 +++++++++++++++---------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 99e5248e73..5d242c4aa4 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -23,29 +23,29 @@ class QcowHeader:
     uint32_t = 'I'
     uint64_t = 'Q'
 
-    fields = [
+    fields = (
         # Version 2 header fields
-        [uint32_t, '%#x',  'magic'],
-        [uint32_t, '%d',   'version'],
-        [uint64_t, '%#x',  'backing_file_offset'],
-        [uint32_t, '%#x',  'backing_file_size'],
-        [uint32_t, '%d',   'cluster_bits'],
-        [uint64_t, '%d',   'size'],
-        [uint32_t, '%d',   'crypt_method'],
-        [uint32_t, '%d',   'l1_size'],
-        [uint64_t, '%#x',  'l1_table_offset'],
-        [uint64_t, '%#x',  'refcount_table_offset'],
-        [uint32_t, '%d',   'refcount_table_clusters'],
-        [uint32_t, '%d',   'nb_snapshots'],
-        [uint64_t, '%#x',  'snapshot_offset'],
+        (uint32_t, '%#x',  'magic'),
+        (uint32_t, '%d',   'version'),
+        (uint64_t, '%#x',  'backing_file_offset'),
+        (uint32_t, '%#x',  'backing_file_size'),
+        (uint32_t, '%d',   'cluster_bits'),
+        (uint64_t, '%d',   'size'),
+        (uint32_t, '%d',   'crypt_method'),
+        (uint32_t, '%d',   'l1_size'),
+        (uint64_t, '%#x',  'l1_table_offset'),
+        (uint64_t, '%#x',  'refcount_table_offset'),
+        (uint32_t, '%d',   'refcount_table_clusters'),
+        (uint32_t, '%d',   'nb_snapshots'),
+        (uint64_t, '%#x',  'snapshot_offset'),
 
         # Version 3 header fields
-        [uint64_t, 'mask', 'incompatible_features'],
-        [uint64_t, 'mask', 'compatible_features'],
-        [uint64_t, 'mask', 'autoclear_features'],
-        [uint32_t, '%d',   'refcount_order'],
-        [uint32_t, '%d',   'header_length'],
-    ]
+        (uint64_t, 'mask', 'incompatible_features'),
+        (uint64_t, 'mask', 'compatible_features'),
+        (uint64_t, 'mask', 'autoclear_features'),
+        (uint32_t, '%d',   'refcount_order'),
+        (uint32_t, '%d',   'header_length'),
+    )
 
     fmt = '>' + ''.join(field[0] for field in fields)
 
-- 
2.21.0

Re: [PATCH v4 04/12] qcow2_format.py: use tuples instead of lists for fields

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 04/12] qcow2_format.py: use tuples instead of lists for fields

No need in lists: it's a constant variable.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 40 +++++++++++++++---------------
 1 file changed, 20 insertions(+), 20 deletions(-)

[-- Attachment #2: Type: text/html, Size: 1828 bytes --]

Re: [PATCH v4 04/12] qcow2_format.py: use tuples instead of lists for fields

[Eric Blake]

On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> No need in lists: it's a constant variable.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2_format.py | 40 +++++++++++++++---------------
>   1 file changed, 20 insertions(+), 20 deletions(-)
> 

>   
>           # Version 3 header fields
> -        [uint64_t, 'mask', 'incompatible_features'],
> -        [uint64_t, 'mask', 'compatible_features'],
> -        [uint64_t, 'mask', 'autoclear_features'],
> -        [uint32_t, '%d',   'refcount_order'],
> -        [uint32_t, '%d',   'header_length'],
> -    ]
> +        (uint64_t, 'mask', 'incompatible_features'),
> +        (uint64_t, 'mask', 'compatible_features'),
> +        (uint64_t, 'mask', 'autoclear_features'),
> +        (uint32_t, '%d',   'refcount_order'),
> +        (uint32_t, '%d',   'header_length'),
> +    )

Not for this patch, but noticing it since we're here: should this be 
taught to list the optional compression_type field that we recently 
added after header_length?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v4 04/12] qcow2_format.py: use tuples instead of lists for fields

[Vladimir Sementsov-Ogievskiy]

05.06.2020 23:16, Eric Blake wrote:
> On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
>> No need in lists: it's a constant variable.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/qcow2_format.py | 40 +++++++++++++++---------------
>>   1 file changed, 20 insertions(+), 20 deletions(-)
>>
> 
>>           # Version 3 header fields
>> -        [uint64_t, 'mask', 'incompatible_features'],
>> -        [uint64_t, 'mask', 'compatible_features'],
>> -        [uint64_t, 'mask', 'autoclear_features'],
>> -        [uint32_t, '%d',   'refcount_order'],
>> -        [uint32_t, '%d',   'header_length'],
>> -    ]
>> +        (uint64_t, 'mask', 'incompatible_features'),
>> +        (uint64_t, 'mask', 'compatible_features'),
>> +        (uint64_t, 'mask', 'autoclear_features'),
>> +        (uint32_t, '%d',   'refcount_order'),
>> +        (uint32_t, '%d',   'header_length'),
>> +    )
> 
> Not for this patch, but noticing it since we're here: should this be taught to list the optional compression_type field that we recently added after header_length?
> 

Good note. And we can use it in corresponding iotest about compression_type. We just should carefully check, how header_length field is handled and fix if needed. I think, Andrey or I will make an additional patch.

-- 
Best regards,
Vladimir

[PATCH v4 05/12] qcow2_format.py: use modern string formatting

[Vladimir Sementsov-Ogievskiy]

Use .format and f-strings instead of old %style. Also, the file uses
both '' and "" quotes, for consistency let's use '', except for cases
when we need '' inside the string (use "" to avoid extra escaping).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 54 +++++++++++++++---------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 5d242c4aa4..80e7c3f81d 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -7,7 +7,7 @@ class QcowHeaderExtension:
     def __init__(self, magic, length, data):
         if length % 8 != 0:
             padding = 8 - (length % 8)
-            data += b"\0" * padding
+            data += b'\0' * padding
 
         self.magic = magic
         self.length = length
@@ -25,26 +25,26 @@ class QcowHeader:
 
     fields = (
         # Version 2 header fields
-        (uint32_t, '%#x',  'magic'),
-        (uint32_t, '%d',   'version'),
-        (uint64_t, '%#x',  'backing_file_offset'),
-        (uint32_t, '%#x',  'backing_file_size'),
-        (uint32_t, '%d',   'cluster_bits'),
-        (uint64_t, '%d',   'size'),
-        (uint32_t, '%d',   'crypt_method'),
-        (uint32_t, '%d',   'l1_size'),
-        (uint64_t, '%#x',  'l1_table_offset'),
-        (uint64_t, '%#x',  'refcount_table_offset'),
-        (uint32_t, '%d',   'refcount_table_clusters'),
-        (uint32_t, '%d',   'nb_snapshots'),
-        (uint64_t, '%#x',  'snapshot_offset'),
+        (uint32_t, '{:#x}', 'magic'),
+        (uint32_t, '{}', 'version'),
+        (uint64_t, '{:#x}', 'backing_file_offset'),
+        (uint32_t, '{:#x}', 'backing_file_size'),
+        (uint32_t, '{}', 'cluster_bits'),
+        (uint64_t, '{}', 'size'),
+        (uint32_t, '{}', 'crypt_method'),
+        (uint32_t, '{}', 'l1_size'),
+        (uint64_t, '{:#x}', 'l1_table_offset'),
+        (uint64_t, '{:#x}', 'refcount_table_offset'),
+        (uint32_t, '{}', 'refcount_table_clusters'),
+        (uint32_t, '{}', 'nb_snapshots'),
+        (uint64_t, '{:#x}', 'snapshot_offset'),
 
         # Version 3 header fields
         (uint64_t, 'mask', 'incompatible_features'),
         (uint64_t, 'mask', 'compatible_features'),
         (uint64_t, 'mask', 'autoclear_features'),
-        (uint32_t, '%d',   'refcount_order'),
-        (uint32_t, '%d',   'header_length'),
+        (uint32_t, '{}', 'refcount_order'),
+        (uint32_t, '{}', 'header_length'),
     )
 
     fmt = '>' + ''.join(field[0] for field in fields)
@@ -102,7 +102,7 @@ class QcowHeader:
 
         fd.seek(self.header_length)
         extensions = self.extensions
-        extensions.append(QcowHeaderExtension(0, 0, b""))
+        extensions.append(QcowHeaderExtension(0, 0, b''))
         for ex in extensions:
             buf = struct.pack('>II', ex.magic, ex.length)
             fd.write(buf)
@@ -113,7 +113,7 @@ class QcowHeader:
             fd.write(self.backing_file)
 
         if fd.tell() > self.cluster_size:
-            raise Exception("I think I just broke the image...")
+            raise Exception('I think I just broke the image...')
 
     def update(self, fd):
         header_bytes = self.header_length
@@ -136,21 +136,21 @@ class QcowHeader:
                         bits.append(bit)
                 value_str = str(bits)
             else:
-                value_str = f[1] % value
+                value_str = f[1].format(value)
 
-            print("%-25s" % f[2], value_str)
+            print(f'{f[2]:<25} {value_str}')
 
     def dump_extensions(self):
         for ex in self.extensions:
 
             data = ex.data[:ex.length]
             if all(c in string.printable.encode('ascii') for c in data):
-                data = "'%s'" % data.decode('ascii')
+                data = f"'{ data.decode('ascii') }'"
             else:
-                data = "<binary>"
+                data = '<binary>'
 
-            print("Header extension:")
-            print("%-25s %#x" % ("magic", ex.magic))
-            print("%-25s %d" % ("length", ex.length))
-            print("%-25s %s" % ("data", data))
-            print("")
+            print('Header extension:')
+            print(f'{"magic":<25} {ex.magic:#x}')
+            print(f'{"length":<25} {ex.length}')
+            print(f'{"data":<25} {data}')
+            print()
-- 
2.21.0

Re: [PATCH v4 05/12] qcow2_format.py: use modern string formatting

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 05/12] qcow2_format.py: use modern string formatting

Use .format and f-strings instead of old %style. Also, the file uses
both '' and "" quotes, for consistency let's use '', except for cases
when we need '' inside the string (use "" to avoid extra escaping).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 54 +++++++++++++++---------------
 1 file changed, 27 insertions(+), 27 deletions(-)

[-- Attachment #2: Type: text/html, Size: 2011 bytes --]

[PATCH v4 06/12] qcow2_format.py: use strings to specify c-type of struct fields

[Vladimir Sementsov-Ogievskiy]

We are going to move field-parsing to super-class, this will be simpler
with simple string specifiers instead of variables.

For some reason python doesn't allow to define ctypes in class too, as
well as fields: it's not available than in 'for' operator. Don't worry:
ctypes will be moved to metaclass soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 50 +++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 80e7c3f81d..1fd9473b7f 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -18,36 +18,42 @@ class QcowHeaderExtension:
         return QcowHeaderExtension(magic, len(data), data)
 
 
-class QcowHeader:
+# Mapping from c types to python struct format
+ctypes = {
+    'u8': 'B',
+    'u16': 'H',
+    'u32': 'I',
+    'u64': 'Q'
+}
+
 
-    uint32_t = 'I'
-    uint64_t = 'Q'
+class QcowHeader:
 
     fields = (
         # Version 2 header fields
-        (uint32_t, '{:#x}', 'magic'),
-        (uint32_t, '{}', 'version'),
-        (uint64_t, '{:#x}', 'backing_file_offset'),
-        (uint32_t, '{:#x}', 'backing_file_size'),
-        (uint32_t, '{}', 'cluster_bits'),
-        (uint64_t, '{}', 'size'),
-        (uint32_t, '{}', 'crypt_method'),
-        (uint32_t, '{}', 'l1_size'),
-        (uint64_t, '{:#x}', 'l1_table_offset'),
-        (uint64_t, '{:#x}', 'refcount_table_offset'),
-        (uint32_t, '{}', 'refcount_table_clusters'),
-        (uint32_t, '{}', 'nb_snapshots'),
-        (uint64_t, '{:#x}', 'snapshot_offset'),
+        ('u32', '{:#x}', 'magic'),
+        ('u32', '{}', 'version'),
+        ('u64', '{:#x}', 'backing_file_offset'),
+        ('u32', '{:#x}', 'backing_file_size'),
+        ('u32', '{}', 'cluster_bits'),
+        ('u64', '{}', 'size'),
+        ('u32', '{}', 'crypt_method'),
+        ('u32', '{}', 'l1_size'),
+        ('u64', '{:#x}', 'l1_table_offset'),
+        ('u64', '{:#x}', 'refcount_table_offset'),
+        ('u32', '{}', 'refcount_table_clusters'),
+        ('u32', '{}', 'nb_snapshots'),
+        ('u64', '{:#x}', 'snapshot_offset'),
 
         # Version 3 header fields
-        (uint64_t, 'mask', 'incompatible_features'),
-        (uint64_t, 'mask', 'compatible_features'),
-        (uint64_t, 'mask', 'autoclear_features'),
-        (uint32_t, '{}', 'refcount_order'),
-        (uint32_t, '{}', 'header_length'),
+        ('u64', 'mask', 'incompatible_features'),
+        ('u64', 'mask', 'compatible_features'),
+        ('u64', 'mask', 'autoclear_features'),
+        ('u32', '{}', 'refcount_order'),
+        ('u32', '{}', 'header_length'),
     )
 
-    fmt = '>' + ''.join(field[0] for field in fields)
+    fmt = '>' + ''.join(ctypes[f[0]] for f in fields)
 
     def __init__(self, fd):
 
-- 
2.21.0

Re: [PATCH v4 06/12] qcow2_format.py: use strings to specify c-type of struct fields

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 06/12] qcow2_format.py: use strings to specify c-type of struct fields

We are going to move field-parsing to super-class, this will be simpler
with simple string specifiers instead of variables.

For some reason python doesn't allow to define ctypes in class too, as
well as fields: it's not available than in 'for' operator. Don't worry:
ctypes will be moved to metaclass soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 50 +++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

[-- Attachment #2: Type: text/html, Size: 2133 bytes --]

Re: [PATCH v4 06/12] qcow2_format.py: use strings to specify c-type of struct fields

[Eric Blake]

On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to move field-parsing to super-class, this will be simpler
> with simple string specifiers instead of variables.
> 
> For some reason python doesn't allow to define ctypes in class too, as
> well as fields: it's not available than in 'for' operator. Don't worry:
> ctypes will be moved to metaclass soon.

Grammar suggestion:
For some reason, python doesn't allow the definition of ctypes in the 
class alongside fields: it would not be available then for use by the 
'for' operator.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2_format.py | 50 +++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 22 deletions(-)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v4 07/12] qcow2_format.py: separate generic functionality of structure classes

[Vladimir Sementsov-Ogievskiy]

We are going to introduce more Qcow2 structure types, defined like
QcowHeader. Move generic functionality into base class to be reused for
further structure classes.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 93 +++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 35 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 1fd9473b7f..d71f578377 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -2,6 +2,62 @@ import struct
 import string
 
 
+class Qcow2StructMeta(type):
+
+    # Mapping from c types to python struct format
+    ctypes = {
+        'u8': 'B',
+        'u16': 'H',
+        'u32': 'I',
+        'u64': 'Q'
+    }
+
+    def __init__(self, name, bases, attrs):
+        if 'fields' in attrs:
+            self.fmt = '>' + ''.join(self.ctypes[f[0]] for f in self.fields)
+
+
+class Qcow2Struct(metaclass=Qcow2StructMeta):
+
+    """Qcow2Struct: base class for qcow2 data structures
+
+    Successors should define fields class variable, which is: list of tuples,
+    each of three elements:
+        - c-type (one of 'u32', 'u64')
+        - format (format_spec to use with .format() when dump or 'mask' to dump
+                  bitmasks)
+        - field name
+    """
+
+    def __init__(self, fd=None, offset=None, data=None):
+        if data is None:
+            assert fd is not None
+            buf_size = struct.calcsize(self.fmt)
+            if offset is not None:
+                fd.seek(offset)
+            data = fd.read(buf_size)
+        else:
+            assert fd is None and offset is None
+
+        values = struct.unpack(self.fmt, data)
+        self.__dict__ = dict((field[2], values[i])
+                             for i, field in enumerate(self.fields))
+
+    def dump(self):
+        for f in self.fields:
+            value = self.__dict__[f[2]]
+            if f[1] == 'mask':
+                bits = []
+                for bit in range(64):
+                    if value & (1 << bit):
+                        bits.append(bit)
+                value_str = str(bits)
+            else:
+                value_str = f[1].format(value)
+
+            print('{:<25} {}'.format(f[2], value_str))
+
+
 class QcowHeaderExtension:
 
     def __init__(self, magic, length, data):
@@ -18,16 +74,7 @@ class QcowHeaderExtension:
         return QcowHeaderExtension(magic, len(data), data)
 
 
-# Mapping from c types to python struct format
-ctypes = {
-    'u8': 'B',
-    'u16': 'H',
-    'u32': 'I',
-    'u64': 'Q'
-}
-
-
-class QcowHeader:
+class QcowHeader(Qcow2Struct):
 
     fields = (
         # Version 2 header fields
@@ -53,18 +100,8 @@ class QcowHeader:
         ('u32', '{}', 'header_length'),
     )
 
-    fmt = '>' + ''.join(ctypes[f[0]] for f in fields)
-
     def __init__(self, fd):
-
-        buf_size = struct.calcsize(QcowHeader.fmt)
-
-        fd.seek(0)
-        buf = fd.read(buf_size)
-
-        header = struct.unpack(QcowHeader.fmt, buf)
-        self.__dict__ = dict((field[2], header[i])
-                             for i, field in enumerate(QcowHeader.fields))
+        super().__init__(fd=fd, offset=0)
 
         self.set_defaults()
         self.cluster_size = 1 << self.cluster_bits
@@ -132,20 +169,6 @@ class QcowHeader:
         buf = buf[0:header_bytes-1]
         fd.write(buf)
 
-    def dump(self):
-        for f in QcowHeader.fields:
-            value = self.__dict__[f[2]]
-            if f[1] == 'mask':
-                bits = []
-                for bit in range(64):
-                    if value & (1 << bit):
-                        bits.append(bit)
-                value_str = str(bits)
-            else:
-                value_str = f[1].format(value)
-
-            print(f'{f[2]:<25} {value_str}')
-
     def dump_extensions(self):
         for ex in self.extensions:
 
-- 
2.21.0

Re: [PATCH v4 07/12] qcow2_format.py: separate generic functionality of structure classes

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

Two more int types in the comment?

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 07/12] qcow2_format.py: separate generic functionality of structure classes

We are going to introduce more Qcow2 structure types, defined like
QcowHeader. Move generic functionality into base class to be reused for
further structure classes.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 93 +++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 35 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 1fd9473b7f..d71f578377 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -2,6 +2,62 @@ import struct

+
+class Qcow2Struct(metaclass=Qcow2StructMeta):
+
+    """Qcow2Struct: base class for qcow2 data structures
+
+    Successors should define fields class variable, which is: list of tuples,
+    each of three elements:
+        - c-type (one of 'u32', 'u64')

u8, u16 ?

[-- Attachment #2: Type: text/html, Size: 3369 bytes --]

Re: [PATCH v4 07/12] qcow2_format.py: separate generic functionality of structure classes

[Vladimir Sementsov-Ogievskiy]

05.06.2020 15:29, Andrey Shinkevich wrote:
> Two more int types in the comment?
> 
> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> *Sent:* Thursday, June 4, 2020 8:41 PM
> *To:* qemu-block@nongnu.org <qemu-block@nongnu.org>
> *Cc:* qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> *Subject:* [PATCH v4 07/12] qcow2_format.py: separate generic functionality of structure classes
> We are going to introduce more Qcow2 structure types, defined like
> QcowHeader. Move generic functionality into base class to be reused for
> further structure classes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2_format.py | 93 +++++++++++++++++++-----------
>   1 file changed, 58 insertions(+), 35 deletions(-)
> 
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index 1fd9473b7f..d71f578377 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -2,6 +2,62 @@ import struct
> 
> +
> +class Qcow2Struct(metaclass=Qcow2StructMeta):
> +
> +    """Qcow2Struct: base class for qcow2 data structures
> +
> +    Successors should define fields class variable, which is: list of tuples,
> +    each of three elements:
> +        - c-type (one of 'u32', 'u64')
> 
> u8, u16 ?

yes


-- 
Best regards,
Vladimir

[PATCH v4 08/12] qcow2_format.py: add field-formatting class

[Vladimir Sementsov-Ogievskiy]

Allow formatter class in structure definition instead of hacking with
'mask'. This will simplify further introduction of new formatters.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 35 +++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index d71f578377..ceb09bde5a 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -2,6 +2,25 @@ import struct
 import string
 
 
+class Qcow2Field:
+
+    def __init__(self, value):
+        self.value = value
+
+    def __str__(self):
+        return str(self.value)
+
+
+class Flags64(Qcow2Field):
+
+    def __str__(self):
+        bits = []
+        for bit in range(64):
+            if self.value & (1 << bit):
+                bits.append(bit)
+        return str(bits)
+
+
 class Qcow2StructMeta(type):
 
     # Mapping from c types to python struct format
@@ -46,14 +65,10 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
     def dump(self):
         for f in self.fields:
             value = self.__dict__[f[2]]
-            if f[1] == 'mask':
-                bits = []
-                for bit in range(64):
-                    if value & (1 << bit):
-                        bits.append(bit)
-                value_str = str(bits)
-            else:
+            if isinstance(f[1], str):
                 value_str = f[1].format(value)
+            else:
+                value_str = str(f[1](value))
 
             print('{:<25} {}'.format(f[2], value_str))
 
@@ -93,9 +108,9 @@ class QcowHeader(Qcow2Struct):
         ('u64', '{:#x}', 'snapshot_offset'),
 
         # Version 3 header fields
-        ('u64', 'mask', 'incompatible_features'),
-        ('u64', 'mask', 'compatible_features'),
-        ('u64', 'mask', 'autoclear_features'),
+        ('u64', Flags64, 'incompatible_features'),
+        ('u64', Flags64, 'compatible_features'),
+        ('u64', Flags64, 'autoclear_features'),
         ('u32', '{}', 'refcount_order'),
         ('u32', '{}', 'header_length'),
     )
-- 
2.21.0

Re: [PATCH v4 08/12] qcow2_format.py: add field-formatting class

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 08/12] qcow2_format.py: add field-formatting class

Allow formatter class in structure definition instead of hacking with
'mask'. This will simplify further introduction of new formatters.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 35 +++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 10 deletions(-)

[-- Attachment #2: Type: text/html, Size: 1936 bytes --]

[PATCH v4 09/12] qcow2_format.py: QcowHeaderExtension: add dump method

[Vladimir Sementsov-Ogievskiy]

Obviously, for-loop body in dump_extensions should be the dump method
of extension.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index ceb09bde5a..ffc7c35b18 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -84,6 +84,17 @@ class QcowHeaderExtension:
         self.length = length
         self.data = data
 
+    def dump(self):
+        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'{"magic":<25} {self.magic:#x}')
+        print(f'{"length":<25} {self.length}')
+        print(f'{"data":<25} {data}')
+
     @classmethod
     def create(cls, magic, data):
         return QcowHeaderExtension(magic, len(data), data)
@@ -186,15 +197,6 @@ class QcowHeader(Qcow2Struct):
 
     def dump_extensions(self):
         for ex in self.extensions:
-
-            data = ex.data[:ex.length]
-            if all(c in string.printable.encode('ascii') for c in data):
-                data = f"'{ data.decode('ascii') }'"
-            else:
-                data = '<binary>'
-
             print('Header extension:')
-            print(f'{"magic":<25} {ex.magic:#x}')
-            print(f'{"length":<25} {ex.length}')
-            print(f'{"data":<25} {data}')
+            ex.dump()
             print()
-- 
2.21.0

Re: [PATCH v4 09/12] qcow2_format.py: QcowHeaderExtension: add dump method

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 09/12] qcow2_format.py: QcowHeaderExtension: add dump method

Obviously, for-loop body in dump_extensions should be the dump method
of extension.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

[-- Attachment #2: Type: text/html, Size: 1849 bytes --]

[PATCH v4 10/12] qcow2_format: refactor QcowHeaderExtension as a subclass of Qcow2Struct

[Vladimir Sementsov-Ogievskiy]

Only two fields we can parse by generic code, but that is better than
nothing. Keep further refactoring of variable-length fields for another
day.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 51 ++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index ffc7c35b18..4683b1e436 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -73,16 +73,39 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
             print('{:<25} {}'.format(f[2], value_str))
 
 
-class QcowHeaderExtension:
+class QcowHeaderExtension(Qcow2Struct):
 
-    def __init__(self, magic, length, data):
-        if length % 8 != 0:
-            padding = 8 - (length % 8)
-            data += b'\0' * padding
+    fields = (
+        ('u32', '{:#x}', 'magic'),
+        ('u32', '{}', 'length')
+        # length bytes of data follows
+        # then padding to next multiply of 8
+    )
 
-        self.magic = magic
-        self.length = length
-        self.data = data
+    def __init__(self, magic=None, length=None, data=None, fd=None):
+        """
+        Support both loading from fd and creation from user data.
+
+        This should be somehow refactored and functionality should be moved to
+        superclass (to allow creation of any qcow2 struct), but then, fields
+        of variable length (data here) should be supported in base class
+        somehow. So, it's a TODO. We'll see how to properly refactor this when
+        we have more qcow2 structures.
+        """
+        if fd is None:
+            assert all(v is not None for v in (magic, length, data))
+            self.magic = magic
+            self.length = length
+            if length % 8 != 0:
+                padding = 8 - (length % 8)
+                data += b'\0' * padding
+            self.data = data
+        else:
+            assert all(v is None for v in (magic, length, data))
+            super().__init__(fd=fd)
+            padded = (self.length + 7) & ~7
+            self.data = fd.read(padded)
+            assert self.data is not None
 
     def dump(self):
         data = self.data[:self.length]
@@ -91,8 +114,7 @@ class QcowHeaderExtension:
         else:
             data = '<binary>'
 
-        print(f'{"magic":<25} {self.magic:#x}')
-        print(f'{"length":<25} {self.length}')
+        super().dump()
         print(f'{"data":<25} {data}')
 
     @classmethod
@@ -158,14 +180,11 @@ class QcowHeader(Qcow2Struct):
             end = self.cluster_size
 
         while fd.tell() < end:
-            (magic, length) = struct.unpack('>II', fd.read(8))
-            if magic == 0:
+            ext = QcowHeaderExtension(fd=fd)
+            if ext.magic == 0:
                 break
             else:
-                padded = (length + 7) & ~7
-                data = fd.read(padded)
-                self.extensions.append(QcowHeaderExtension(magic, length,
-                                                           data))
+                self.extensions.append(ext)
 
     def update_extensions(self, fd):
 
-- 
2.21.0

Re: [PATCH v4 10/12] qcow2_format: refactor QcowHeaderExtension as a subclass of Qcow2Struct

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

I'd add a comment that correct current file offset is the responsibility of a caller.

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 10/12] qcow2_format: refactor QcowHeaderExtension as a subclass of Qcow2Struct

Only two fields we can parse by generic code, but that is better than
nothing. Keep further refactoring of variable-length fields for another
day.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/qcow2_format.py | 51 ++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 16 deletions(-)

[-- Attachment #2: Type: text/html, Size: 2098 bytes --]

[PATCH v4 11/12] qcow2: QcowHeaderExtension print names for extension magics

[Vladimir Sementsov-Ogievskiy]

Suggested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/031.out         | 22 +++++++++++-----------
 tests/qemu-iotests/036.out         |  4 ++--
 tests/qemu-iotests/061.out         | 14 +++++++-------
 tests/qemu-iotests/qcow2_format.py | 17 ++++++++++++++++-
 4 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index 5a4beda6a2..e0225f4247 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -25,7 +25,7 @@ refcount_order            4
 header_length             72
 
 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'
 
@@ -53,7 +53,7 @@ refcount_order            4
 header_length             72
 
 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'
 
@@ -81,12 +81,12 @@ refcount_order            4
 header_length             72
 
 Header extension:
-magic                     0xe2792aca
+magic                     3799591626 (Backing format)
 length                    11
 data                      'host_device'
 
 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'
 
@@ -116,12 +116,12 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'
 
@@ -149,12 +149,12 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'
 
@@ -182,17 +182,17 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0xe2792aca
+magic                     3799591626 (Backing format)
 length                    11
 data                      'host_device'
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'
 
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index e409acf60e..6cf892123a 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -25,7 +25,7 @@ incompatible_features     []
 compatible_features       []
 autoclear_features        [63]
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -37,7 +37,7 @@ incompatible_features     []
 compatible_features       []
 autoclear_features        []
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index a51ad1b5ba..7afd2be9d9 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -25,7 +25,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -83,7 +83,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -139,7 +139,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -194,7 +194,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -263,7 +263,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -325,7 +325,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
@@ -354,7 +354,7 @@ refcount_order            4
 header_length             112
 
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>
 
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 4683b1e436..caa190f7b1 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -21,6 +21,12 @@ class Flags64(Qcow2Field):
         return str(bits)
 
 
+class Enum(Qcow2Field):
+
+    def __str__(self):
+        return f'{self.value} ({self.mapping.get(self.value, "<unknown>")})'
+
+
 class Qcow2StructMeta(type):
 
     # Mapping from c types to python struct format
@@ -75,8 +81,17 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
 
 class QcowHeaderExtension(Qcow2Struct):
 
+    class Magic(Enum):
+        mapping = {
+            0xE2792ACA: 'Backing format',
+            0x6803f857: 'Feature table',
+            0x0537be77: 'Crypto header',
+            0x23852875: 'Bitmaps',
+            0x44415441: 'Data file'
+        }
+
     fields = (
-        ('u32', '{:#x}', 'magic'),
+        ('u32', Magic, 'magic'),
         ('u32', '{}', 'length')
         # length bytes of data follows
         # then padding to next multiply of 8
-- 
2.21.0

Re: [PATCH v4 11/12] qcow2: QcowHeaderExtension print names for extension magics

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 7891 bytes --]

s/0xE2792ACA/0xe2792aca/
(as suggested by Eric)

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 11/12] qcow2: QcowHeaderExtension print names for extension magics

Suggested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/031.out         | 22 +++++++++++-----------
 tests/qemu-iotests/036.out         |  4 ++--
 tests/qemu-iotests/061.out         | 14 +++++++-------
 tests/qemu-iotests/qcow2_format.py | 17 ++++++++++++++++-
 4 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index 5a4beda6a2..e0225f4247 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -25,7 +25,7 @@ refcount_order            4
 header_length             72

 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'

@@ -53,7 +53,7 @@ refcount_order            4
 header_length             72

 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'

@@ -81,12 +81,12 @@ refcount_order            4
 header_length             72

 Header extension:
-magic                     0xe2792aca
+magic                     3799591626 (Backing format)
 length                    11
 data                      'host_device'

 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'

@@ -116,12 +116,12 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'

@@ -149,12 +149,12 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'

@@ -182,17 +182,17 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0xe2792aca
+magic                     3799591626 (Backing format)
 length                    11
 data                      'host_device'

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

 Header extension:
-magic                     0x12345678
+magic                     305419896 (<unknown>)
 length                    31
 data                      'This is a test header extension'

diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index e409acf60e..6cf892123a 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -25,7 +25,7 @@ incompatible_features     []
 compatible_features       []
 autoclear_features        [63]
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -37,7 +37,7 @@ incompatible_features     []
 compatible_features       []
 autoclear_features        []
 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index a51ad1b5ba..7afd2be9d9 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -25,7 +25,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -83,7 +83,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -139,7 +139,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -194,7 +194,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -263,7 +263,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -325,7 +325,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

@@ -354,7 +354,7 @@ refcount_order            4
 header_length             112

 Header extension:
-magic                     0x6803f857
+magic                     1745090647 (Feature table)
 length                    336
 data                      <binary>

diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 4683b1e436..caa190f7b1 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -21,6 +21,12 @@ class Flags64(Qcow2Field):
         return str(bits)


+class Enum(Qcow2Field):
+
+    def __str__(self):
+        return f'{self.value} ({self.mapping.get(self.value, "<unknown>")})'
+
+
 class Qcow2StructMeta(type):

     # Mapping from c types to python struct format
@@ -75,8 +81,17 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):

 class QcowHeaderExtension(Qcow2Struct):

+    class Magic(Enum):
+        mapping = {
+            0xE2792ACA: 'Backing format',
+            0x6803f857: 'Feature table',
+            0x0537be77: 'Crypto header',
+            0x23852875: 'Bitmaps',
+            0x44415441: 'Data file'
+        }
+
     fields = (
-        ('u32', '{:#x}', 'magic'),
+        ('u32', Magic, 'magic'),
         ('u32', '{}', 'length')
         # length bytes of data follows
         # then padding to next multiply of 8
--
2.21.0


[-- Attachment #2: Type: text/html, Size: 22038 bytes --]

Re: [PATCH v4 11/12] qcow2: QcowHeaderExtension print names for extension magics

[Vladimir Sementsov-Ogievskiy]

04.06.2020 20:41, Vladimir Sementsov-Ogievskiy wrote:
> Suggested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/031.out         | 22 +++++++++++-----------
>   tests/qemu-iotests/036.out         |  4 ++--
>   tests/qemu-iotests/061.out         | 14 +++++++-------
>   tests/qemu-iotests/qcow2_format.py | 17 ++++++++++++++++-
>   4 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
> index 5a4beda6a2..e0225f4247 100644
> --- a/tests/qemu-iotests/031.out
> +++ b/tests/qemu-iotests/031.out
> @@ -25,7 +25,7 @@ refcount_order            4
>   header_length             72
>   
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)

Oops, I've broken hex number. I think it should be kept hex, even with name.

>   length                    31
>   data                      'This is a test header extension'
>   
> @@ -53,7 +53,7 @@ refcount_order            4
>   header_length             72
>   
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)
>   length                    31
>   data                      'This is a test header extension'
>   
> @@ -81,12 +81,12 @@ refcount_order            4
>   header_length             72
>   
>   Header extension:
> -magic                     0xe2792aca
> +magic                     3799591626 (Backing format)
>   length                    11
>   data                      'host_device'
>   
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)
>   length                    31
>   data                      'This is a test header extension'
>   
> @@ -116,12 +116,12 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)
>   length                    31
>   data                      'This is a test header extension'
>   
> @@ -149,12 +149,12 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)
>   length                    31
>   data                      'This is a test header extension'
>   
> @@ -182,17 +182,17 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0xe2792aca
> +magic                     3799591626 (Backing format)
>   length                    11
>   data                      'host_device'
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)
>   length                    31
>   data                      'This is a test header extension'
>   
> diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
> index e409acf60e..6cf892123a 100644
> --- a/tests/qemu-iotests/036.out
> +++ b/tests/qemu-iotests/036.out
> @@ -25,7 +25,7 @@ incompatible_features     []
>   compatible_features       []
>   autoclear_features        [63]
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -37,7 +37,7 @@ incompatible_features     []
>   compatible_features       []
>   autoclear_features        []
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
> index a51ad1b5ba..7afd2be9d9 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -25,7 +25,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -83,7 +83,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -139,7 +139,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -194,7 +194,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -263,7 +263,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -325,7 +325,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> @@ -354,7 +354,7 @@ refcount_order            4
>   header_length             112
>   
>   Header extension:
> -magic                     0x6803f857
> +magic                     1745090647 (Feature table)
>   length                    336
>   data                      <binary>
>   
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index 4683b1e436..caa190f7b1 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -21,6 +21,12 @@ class Flags64(Qcow2Field):
>           return str(bits)
>   
>   
> +class Enum(Qcow2Field):
> +
> +    def __str__(self):
> +        return f'{self.value} ({self.mapping.get(self.value, "<unknown>")})'
> +
> +
>   class Qcow2StructMeta(type):
>   
>       # Mapping from c types to python struct format
> @@ -75,8 +81,17 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>   
>   class QcowHeaderExtension(Qcow2Struct):
>   
> +    class Magic(Enum):
> +        mapping = {
> +            0xE2792ACA: 'Backing format',
> +            0x6803f857: 'Feature table',
> +            0x0537be77: 'Crypto header',
> +            0x23852875: 'Bitmaps',
> +            0x44415441: 'Data file'
> +        }
> +
>       fields = (
> -        ('u32', '{:#x}', 'magic'),
> +        ('u32', Magic, 'magic'),
>           ('u32', '{}', 'length')
>           # length bytes of data follows
>           # then padding to next multiply of 8
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 11/12] qcow2: QcowHeaderExtension print names for extension magics

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

Yes, but hex(int) returns str.

Andrey

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Friday, June 5, 2020 7:26 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: Re: [PATCH v4 11/12] qcow2: QcowHeaderExtension print names for extension magics

04.06.2020 20:41, Vladimir Sementsov-Ogievskiy wrote:
> Suggested-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/031.out         | 22 +++++++++++-----------
>   tests/qemu-iotests/036.out         |  4 ++--
>   tests/qemu-iotests/061.out         | 14 +++++++-------
>   tests/qemu-iotests/qcow2_format.py | 17 ++++++++++++++++-
>   4 files changed, 36 insertions(+), 21 deletions(-)
>
> diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
> index 5a4beda6a2..e0225f4247 100644
> --- a/tests/qemu-iotests/031.out
> +++ b/tests/qemu-iotests/031.out
> @@ -25,7 +25,7 @@ refcount_order            4
>   header_length             72
>
>   Header extension:
> -magic                     0x12345678
> +magic                     305419896 (<unknown>)

Oops, I've broken hex number. I think it should be kept hex, even with name.


[-- Attachment #2: Type: text/html, Size: 2979 bytes --]

[PATCH v4 12/12] qcow2_format.py: dump bitmaps header extension

[Vladimir Sementsov-Ogievskiy]

Add class for bitmap extension and dump its fields. Further work is to
dump bitmap directory.

Test new functionality inside 291 iotest.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/291             |  4 +++
 tests/qemu-iotests/291.out         | 33 +++++++++++++++++++++++
 tests/qemu-iotests/qcow2_format.py | 42 +++++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 3ca83b9cd1..e0cffc7cb1 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -62,6 +62,8 @@ $QEMU_IO -c 'w 1M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG bitmap --disable -f $IMGFMT "$TEST_IMG" b1
 $QEMU_IMG bitmap --enable -f $IMGFMT "$TEST_IMG" b2
 $QEMU_IO -c 'w 2M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io
+echo "Check resulting qcow2 header extensions:"
+$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 echo
 echo "=== Bitmap preservation not possible to non-qcow2 ==="
@@ -88,6 +90,8 @@ $QEMU_IMG bitmap --merge tmp -f $IMGFMT "$TEST_IMG" b0
 $QEMU_IMG bitmap --remove --image-opts \
     driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
 $QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific
+echo "Check resulting qcow2 header extensions:"
+$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 echo
 echo "=== Check bitmap contents ==="
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index 8c62017567..1d4f9cd96d 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -14,6 +14,25 @@ wrote 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1048576/1048576 bytes at offset 2097152
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Check resulting qcow2 header extensions:
+Header extension:
+magic                     3799591626 (Backing format)
+length                    5
+data                      'qcow2'
+
+Header extension:
+magic                     1745090647 (Feature table)
+length                    336
+data                      <binary>
+
+Header extension:
+magic                     595929205 (Bitmaps)
+length                    24
+nb_bitmaps                2
+reserved32                0
+bitmap_directory_size     0x40
+bitmap_directory_offset   0x510000
+
 
 === Bitmap preservation not possible to non-qcow2 ===
 
@@ -65,6 +84,20 @@ Format specific information:
             granularity: 65536
     refcount bits: 16
     corrupt: false
+Check resulting qcow2 header extensions:
+Header extension:
+magic                     1745090647 (Feature table)
+length                    336
+data                      <binary>
+
+Header extension:
+magic                     595929205 (Bitmaps)
+length                    24
+nb_bitmaps                3
+reserved32                0
+bitmap_directory_size     0x60
+bitmap_directory_offset   0x520000
+
 
 === Check bitmap contents ===
 
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index caa190f7b1..c2c636b442 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -79,6 +79,19 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
             print('{:<25} {}'.format(f[2], value_str))
 
 
+class Qcow2BitmapExt(Qcow2Struct):
+
+    fields = (
+        ('u32', '{}', 'nb_bitmaps'),
+        ('u32', '{}', 'reserved32'),
+        ('u64', '{:#x}', 'bitmap_directory_size'),
+        ('u64', '{:#x}', 'bitmap_directory_offset')
+    )
+
+
+QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
+
+
 class QcowHeaderExtension(Qcow2Struct):
 
     class Magic(Enum):
@@ -86,7 +99,7 @@ class QcowHeaderExtension(Qcow2Struct):
             0xE2792ACA: 'Backing format',
             0x6803f857: 'Feature table',
             0x0537be77: 'Crypto header',
-            0x23852875: 'Bitmaps',
+            QCOW2_EXT_MAGIC_BITMAPS: 'Bitmaps',
             0x44415441: 'Data file'
         }
 
@@ -104,8 +117,11 @@ class QcowHeaderExtension(Qcow2Struct):
         This should be somehow refactored and functionality should be moved to
         superclass (to allow creation of any qcow2 struct), but then, fields
         of variable length (data here) should be supported in base class
-        somehow. So, it's a TODO. We'll see how to properly refactor this when
-        we have more qcow2 structures.
+        somehow. Note also, that we probably want to parse different
+        extensions. Should they be subclasses of this class, or how to do it
+        better? Should it be something like QAPI union with discriminator field
+        (magic here). So, it's a TODO. We'll see how to properly refactor this
+        when we have more qcow2 structures.
         """
         if fd is None:
             assert all(v is not None for v in (magic, length, data))
@@ -122,15 +138,23 @@ class QcowHeaderExtension(Qcow2Struct):
             self.data = fd.read(padded)
             assert self.data is not None
 
-    def dump(self):
-        data = self.data[:self.length]
-        if all(c in string.printable.encode('ascii') for c in data):
-            data = f"'{ data.decode('ascii') }'"
+        if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
+            self.obj = Qcow2BitmapExt(data=self.data)
         else:
-            data = '<binary>'
+            self.obj = None
 
+    def dump(self):
         super().dump()
-        print(f'{"data":<25} {data}')
+
+        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}')
+        else:
+            self.obj.dump()
 
     @classmethod
     def create(cls, magic, data):
-- 
2.21.0

Re: [PATCH v4 12/12] qcow2_format.py: dump bitmaps header extension

[Andrey Shinkevich]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, June 4, 2020 8:41 PM
To: qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mreitz@redhat.com <mreitz@redhat.com>; kwolf@redhat.com <kwolf@redhat.com>; eblake@redhat.com <eblake@redhat.com>; Denis Lunev <den@virtuozzo.com>; Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Subject: [PATCH v4 12/12] qcow2_format.py: dump bitmaps header extension

Add class for bitmap extension and dump its fields. Further work is to
dump bitmap directory.

Test new functionality inside 291 iotest.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/291             |  4 +++
 tests/qemu-iotests/291.out         | 33 +++++++++++++++++++++++
 tests/qemu-iotests/qcow2_format.py | 42 +++++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 9 deletions(-)

[-- Attachment #2: Type: text/html, Size: 2296 bytes --]

Re: [PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200604174135.11042-1-vsementsov@virtuozzo.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200604174135.11042-1-vsementsov@virtuozzo.com
Subject: [PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
ff3d0e0 qcow2_format.py: dump bitmaps header extension
1ca4b4b qcow2: QcowHeaderExtension print names for extension magics
4894f85 qcow2_format: refactor QcowHeaderExtension as a subclass of Qcow2Struct
5d2d3ea qcow2_format.py: QcowHeaderExtension: add dump method
7848f52 qcow2_format.py: add field-formatting class
4f0473f qcow2_format.py: separate generic functionality of structure classes
78964d2 qcow2_format.py: use strings to specify c-type of struct fields
492b753 qcow2_format.py: use modern string formatting
7fef737 qcow2_format.py: use tuples instead of lists for fields
8fae4ab qcow2_format.py: drop new line printing at end of dump()
c0b4e4b qcow2.py: move qcow2 format classes to separate module
32668ba qcow2.py: python style fixes

=== OUTPUT BEGIN ===
1/12 Checking commit 32668ba94cf1 (qcow2.py: python style fixes)
ERROR: line over 90 characters
#219: FILE: tests/qemu-iotests/qcow2.py:256:
+    ['dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions'],

WARNING: line over 80 characters
#220: FILE: tests/qemu-iotests/qcow2.py:257:
+    ['dump-header-exts',     cmd_dump_header_exts,     0, 'Dump image header extensions'],

WARNING: line over 80 characters
#221: FILE: tests/qemu-iotests/qcow2.py:258:
+    ['set-header',           cmd_set_header,           2, 'Set a field in the header'],

WARNING: line over 80 characters
#222: FILE: tests/qemu-iotests/qcow2.py:259:
+    ['add-header-ext',       cmd_add_header_ext,       2, 'Add a header extension'],

ERROR: line over 90 characters
#223: FILE: tests/qemu-iotests/qcow2.py:260:
+    ['add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header extension, data from stdin'],

WARNING: line over 80 characters
#224: FILE: tests/qemu-iotests/qcow2.py:261:
+    ['del-header-ext',       cmd_del_header_ext,       1, 'Delete a header extension'],

total: 2 errors, 4 warnings, 217 lines checked

Patch 1/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/12 Checking commit c0b4e4bcc692 (qcow2.py: move qcow2 format classes to separate module)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#186: 
new file mode 100644

total: 0 errors, 1 warnings, 324 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit 8fae4ab6b058 (qcow2_format.py: drop new line printing at end of dump())
4/12 Checking commit 7fef73761e63 (qcow2_format.py: use tuples instead of lists for fields)
5/12 Checking commit 492b753d310e (qcow2_format.py: use modern string formatting)
6/12 Checking commit 78964d206cab (qcow2_format.py: use strings to specify c-type of struct fields)
7/12 Checking commit 4f0473f3cf7f (qcow2_format.py: separate generic functionality of structure classes)
8/12 Checking commit 7848f5283863 (qcow2_format.py: add field-formatting class)
9/12 Checking commit 5d2d3ea44d93 (qcow2_format.py: QcowHeaderExtension: add dump method)
10/12 Checking commit 4894f859e63d (qcow2_format: refactor QcowHeaderExtension as a subclass of Qcow2Struct)
11/12 Checking commit 1ca4b4b795d8 (qcow2: QcowHeaderExtension print names for extension magics)
12/12 Checking commit ff3d0e0f471a (qcow2_format.py: dump bitmaps header extension)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200604174135.11042-1-vsementsov@virtuozzo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata

[Vladimir Sementsov-Ogievskiy]

04.06.2020 22:14, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200604174135.11042-1-vsementsov@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20200604174135.11042-1-vsementsov@virtuozzo.com
> Subject: [PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Switched to a new branch 'test'
> ff3d0e0 qcow2_format.py: dump bitmaps header extension
> 1ca4b4b qcow2: QcowHeaderExtension print names for extension magics
> 4894f85 qcow2_format: refactor QcowHeaderExtension as a subclass of Qcow2Struct
> 5d2d3ea qcow2_format.py: QcowHeaderExtension: add dump method
> 7848f52 qcow2_format.py: add field-formatting class
> 4f0473f qcow2_format.py: separate generic functionality of structure classes
> 78964d2 qcow2_format.py: use strings to specify c-type of struct fields
> 492b753 qcow2_format.py: use modern string formatting
> 7fef737 qcow2_format.py: use tuples instead of lists for fields
> 8fae4ab qcow2_format.py: drop new line printing at end of dump()
> c0b4e4b qcow2.py: move qcow2 format classes to separate module
> 32668ba qcow2.py: python style fixes
> 
> === OUTPUT BEGIN ===
> 1/12 Checking commit 32668ba94cf1 (qcow2.py: python style fixes)
> ERROR: line over 90 characters
> #219: FILE: tests/qemu-iotests/qcow2.py:256:
> +    ['dump-header',          cmd_dump_header,          0, 'Dump image header and header extensions'],
> 
> WARNING: line over 80 characters
> #220: FILE: tests/qemu-iotests/qcow2.py:257:
> +    ['dump-header-exts',     cmd_dump_header_exts,     0, 'Dump image header extensions'],
> 
> WARNING: line over 80 characters
> #221: FILE: tests/qemu-iotests/qcow2.py:258:
> +    ['set-header',           cmd_set_header,           2, 'Set a field in the header'],
> 
> WARNING: line over 80 characters
> #222: FILE: tests/qemu-iotests/qcow2.py:259:
> +    ['add-header-ext',       cmd_add_header_ext,       2, 'Add a header extension'],
> 
> ERROR: line over 90 characters
> #223: FILE: tests/qemu-iotests/qcow2.py:260:
> +    ['add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header extension, data from stdin'],
> 
> WARNING: line over 80 characters
> #224: FILE: tests/qemu-iotests/qcow2.py:261:
> +    ['del-header-ext',       cmd_del_header_ext,       1, 'Delete a header extension'],
> 
> total: 2 errors, 4 warnings, 217 lines checked
> 
> Patch 1/12 has style problems, please review.  If any of these errors


Preexisting. And it will look worse if wrap lines.


-- 
Best regards,
Vladimir

Re: [PATCH v4 00/12] iotests: Dump QCOW2 dirty bitmaps metadata

[Eric Blake]

On 6/4/20 12:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is my suggestion to substitute only first three patches :) of
> Andrey's [PATCH v3 0/6] iotests: Dump QCOW2 dirty bitmaps metadata
> 
> so, I called it v4 for convenience.
> 
> What is here:
> 1. First, update code style
> 2. Next, try to refactor in a manner which will make adding new data
> structures simple (look at Qcow2BitmapExt class in last patch)
> 
> I think, next step is to add type hints. Then add more structures.
> And, anyway, at some point we should move it into python/ directory (at
> least qcow2_format.py lib)

My python reviewing skills are weak, but in general this series makes 
sense.  I had enough comments that you're probably better off spinning a 
v5, but it looks like it is probably close enough that I can include 
this in my next bitmaps pull request, and we can work on rebasing the 
remainder of Andrey's patches on top of it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org