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

[Eric Blake]

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>

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