[v3,4/6] iotests: Dump bitmap directory info with qcow2.py

Read and dump entries from the bitmap directory of QCOW2 image with the
script qcow2.py.

Header extension:         Bitmaps
...
Bitmap name               bitmap-1
flag                      auto
bitmap_table_offset       0xf0000
bitmap_table_size         8
flag_bits                 2
type                      1
granularity_bits          16
name_size                 8
extra_data_size           0

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/qcow2.py | 104 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

On 6/1/20 8:48 AM, Andrey Shinkevich wrote:
> Read and dump entries from the bitmap directory of QCOW2 image with the
> script qcow2.py.
> 
> Header extension:         Bitmaps
> ...
> Bitmap name               bitmap-1
> flag                      auto
> bitmap_table_offset       0xf0000
> bitmap_table_size         8
> flag_bits                 2
> type                      1
> granularity_bits          16
> name_size                 8
> extra_data_size           0
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2.py | 104 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 103 insertions(+), 1 deletion(-)
> 

> +        self.bitmap_flags = []
> +        BME_FLAG_IN_USE = 1
> +        BME_FLAG_AUTO = 1 << 1

Would it be worth using '1 << 0' for BME_FLAG_IN_USE, to make it obvious 
that these are consecutive bits, especially if we later add a third bit?


> +        for n in range(self.nb_bitmaps):
> +            buf = fd.read(buf_size)
> +            dir_entry = Qcow2BitmapDirEntry(buf)
> +            fd.seek(dir_entry.extra_data_size, 1)
> +            bitmap_name = fd.read(dir_entry.name_size)
> +            dir_entry.name = bitmap_name.decode('ascii')
> +            self.bitmaps.append(dir_entry)
> +            entry_raw_size = dir_entry.bitmap_dir_entry_raw_size()
> +            shift = ((entry_raw_size + 7) & ~7) - entry_raw_size
> +            fd.seek(shift, 1)

Is there a symbolic constant instead of the magic '1' for fd.seek()?


> @@ -157,7 +256,10 @@ 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))

Should this reformatting be done earlier in the series to minimize churn?

> +        for ex in self.extensions:
> +            ex.load(fd)
>   
>       def update_extensions(self, fd):
>   
> 

Fixing the things I pointed out does not seem major, so
Reviewed-by: Eric Blake <eblake@redhat.com>

01.06.2020 16:48, Andrey Shinkevich wrote:
> Read and dump entries from the bitmap directory of QCOW2 image with the
> script qcow2.py.
> 
> Header extension:         Bitmaps
> ...
> Bitmap name               bitmap-1
> flag                      auto
> bitmap_table_offset       0xf0000
> bitmap_table_size         8
> flag_bits                 2
> type                      1
> granularity_bits          16
> name_size                 8
> extra_data_size           0
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2.py | 104 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
> index 8286115..e4453f6 100755
> --- a/tests/qemu-iotests/qcow2.py
> +++ b/tests/qemu-iotests/qcow2.py
> @@ -5,6 +5,88 @@ import struct
>   import string
>   
>   
> +class Qcow2BitmapDirEntry:
> +
> +    name = ''
> +
> +    uint8_t = 'B'
> +    uint16_t = 'H'
> +    uint32_t = 'I'
> +    uint64_t = 'Q'
> +
> +    fields = [
> +        [uint64_t, '%#x', 'bitmap_table_offset'],
> +        [uint32_t, '%d',  'bitmap_table_size'],
> +        [uint32_t, '%d',  'flag_bits'],

I'd keep exact field name from spec: "flags"

> +        [uint8_t,  '%d',  'type'],
> +        [uint8_t,  '%d',  'granularity_bits'],
> +        [uint16_t, '%d',  'name_size'],
> +        [uint32_t, '%d',  'extra_data_size']
> +    ]
> +
> +    fmt = '>' + ''.join(field[0] for field in fields)
> +
> +    def __init__(self, data):
> +

You are not consistent about adding this empty line. I'd avoid it.

> +        entry = struct.unpack(Qcow2BitmapDirEntry.fmt, data)
> +        self.__dict__ = dict((field[2], entry[i])
> +                             for i, field in enumerate(
> +                                 Qcow2BitmapDirEntry.fields))
> +
> +        self.bitmap_table_size = self.bitmap_table_size \
> +            * struct.calcsize(self.uint64_t)

I don't like this: you keep name from specification, but change its
meaning.

> +
> +        self.bitmap_flags = []
> +        BME_FLAG_IN_USE = 1
> +        BME_FLAG_AUTO = 1 << 1

I'd define all constants copied from C code in global space, to be simply
available from the module.

> +        if (self.flag_bits & BME_FLAG_IN_USE) != 0:

in python zero is false enough :) (you may omit comparison to 0)

> +            self.bitmap_flags.append("in-use")
> +        if (self.flag_bits & BME_FLAG_AUTO) != 0:
> +            self.bitmap_flags.append("auto")
> +
> +    def bitmap_dir_entry_raw_size(self):
> +        return struct.calcsize(self.fmt) + self.name_size + \
> +            self.extra_data_size
> +
> +    def dump_bitmap_dir_entry(self):
> +        print("%-25s" % 'Bitmap name', self.name)
> +
> +        for fl in self.bitmap_flags:
> +            print("%-25s" % 'flag', fl)
> +
> +        for f in Qcow2BitmapDirEntry.fields:
> +            value = self.__dict__[f[2]]
> +            value_str = f[1] % value
> +            print("%-25s" % f[2], value_str)
> +
> +
> +class Qcow2BitmapDirectory:
> +
> +    def __init__(self, bm_header_ext):
> +        self.nb_bitmaps = bm_header_ext.nb_bitmaps
> +        self.bitmap_directory_offset = bm_header_ext.bitmap_directory_offset
> +        self.bitmap_directory_size = bm_header_ext.bitmap_directory_size

Honestly, I don't like duplicating attributes between different objects.

> +
> +    def read_bitmap_directory(self, fd):

Why not do it in constructor? The only use of the class

> +        self.bitmaps = []
> +        fd.seek(self.bitmap_directory_offset)
> +        buf_size = struct.calcsize(Qcow2BitmapDirEntry.fmt)
> +
> +        for n in range(self.nb_bitmaps):
> +            buf = fd.read(buf_size)
> +            dir_entry = Qcow2BitmapDirEntry(buf)
> +            fd.seek(dir_entry.extra_data_size, 1)
> +            bitmap_name = fd.read(dir_entry.name_size)
> +            dir_entry.name = bitmap_name.decode('ascii')
> +            self.bitmaps.append(dir_entry)
> +            entry_raw_size = dir_entry.bitmap_dir_entry_raw_size()
> +            shift = ((entry_raw_size + 7) & ~7) - entry_raw_size
> +            fd.seek(shift, 1)
> +
> +    def get_bitmaps(self):
> +        return self.bitmaps

Why we need getter for this field?

> +
> +
>   class Qcow2BitmapExt:
>   
>       uint32_t = 'I'
> @@ -33,8 +115,21 @@ class Qcow2BitmapExt:
>               print("%-25s" % f[2], value_str)
>           print("")
>   
> +    def read_bitmap_directory(self, fd):
> +        bm_directory = Qcow2BitmapDirectory(self)
> +        bm_directory.read_bitmap_directory(fd)
> +        self.bitmaps = bm_directory.get_bitmaps()
> +
> +    def load(self, fd):
> +        self.read_bitmap_directory(fd)
> +
> +    def dump_bitmap_directory(self):
> +        for bm in self.bitmaps:
> +            bm.dump_bitmap_dir_entry()
> +
>       def dump_ext(self):
>           self.dump_bitmap_ext()
> +        self.dump_bitmap_directory()
>   
>   
>   class QcowHeaderExtension:
> @@ -79,6 +174,10 @@ class QcowHeaderExtension:
>               self.QCOW2_EXT_MAGIC_DATA_FILE: 'Data file',
>           }.get(magic, 'Unknown')
>   
> +    def load(self, fd):
> +        if self.obj is not None:
> +            self.obj.load(fd)
> +
>   
>   class QcowHeader:
>   
> @@ -157,7 +256,10 @@ 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))
> +        for ex in self.extensions:
> +            ex.load(fd)
>   
>       def update_extensions(self, fd):
>   
>