Re: [PATCH v3 3/6] iotests: dump bitmap extension data with qcow2.py

[Vladimir Sementsov-Ogievskiy]

01.06.2020 16:48, Andrey Shinkevich wrote:
> Add bitmap header extension data, if any, to the dump in qcow2.py.
>
> Header extension:         Bitmaps
> magic                     0x23852875
> length                    24
> nb_bitmaps                2
> reserved32                0
> bitmap_directory_size     0x40
> bitmap_directory_offset   0x100000
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2.py | 42 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
> index 18e4923..8286115 100755
> --- a/tests/qemu-iotests/qcow2.py
> +++ b/tests/qemu-iotests/qcow2.py
> @@ -4,6 +4,39 @@ import sys
>   import struct
>   import string
>   
> +
> +class Qcow2BitmapExt:
> +
> +    uint32_t = 'I'
> +    uint64_t = 'Q'
> +
> +    fields = [
> +        [uint32_t, '%d',  'nb_bitmaps'],
> +        [uint32_t, '%d',  'reserved32'],
> +        [uint64_t, '%#x', 'bitmap_directory_size'],
> +        [uint64_t, '%#x', 'bitmap_directory_offset']
> +    ]
> +
> +    fmt = '>' + ''.join(field[0] for field in fields)
> +
> +    def __init__(self, data):
> +
> +        extension = struct.unpack(Qcow2BitmapExt.fmt, data)
> +        self.__dict__ = dict((field[2], extension[i])
> +                             for i, field in enumerate(Qcow2BitmapExt.fields))
> +
> +    def dump_bitmap_ext(self):
> +        for f in Qcow2BitmapExt.fields:
> +            value = self.__dict__[f[2]]
> +            value_str = f[1] % value
> +
> +            print("%-25s" % f[2], value_str)
> +        print("")
> +
> +    def dump_ext(self):
> +        self.dump_bitmap_ext()

Hmm, don't see, why this can't be one function named "dump".

Still, I do think it should be moved to parent class of both Qcow2BitmapExt and 
QcowHeader,
I'll send refactoring follow-up, so it doesn't really matter now.

> +
> +
>   class QcowHeaderExtension:
>   
>       QCOW2_EXT_MAGIC_BACKING_FORMAT = 0xE2792ACA
> @@ -13,12 +46,16 @@ class QcowHeaderExtension:
>       QCOW2_EXT_MAGIC_DATA_FILE = 0x44415441
>   
>       def __init__(self, magic, length, data):
> +        self.obj = None
>           data_str = data[:length]
>           if all(c in string.printable.encode('ascii') for c in data_str):
>               data_str = "'%s'" % data_str.decode('ascii')
>           else:
>               data_str = "<binary>"
>   
> +        if magic == self.QCOW2_EXT_MAGIC_BITMAPS:
> +            self.obj = Qcow2BitmapExt(data)
> +
>           if length % 8 != 0:
>               padding = 8 - (length % 8)
>               data += b"\0" * padding
> @@ -172,7 +209,10 @@ class QcowHeader:
>               print("%-25s %s" % ("Header extension:", ex.name))
>               print("%-25s %#x" % ("magic", ex.magic))
>               print("%-25s %d" % ("length", ex.length))
> -            print("%-25s %s" % ("data", ex.data_str))
> +            if ex.obj is not None:
> +                ex.obj.dump_ext()
> +            else:
> +                print("%-25s %s" % ("data", ex.data_str))

this change make it more obvious that this should be method of 
QcowHeaderExtension.

Still, this works as is, nothing serious:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


--
Best regards,
Vladimir