Re: [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: kwolf@redhat.com, den@openvz.org, qemu-devel@nongnu.org,
	mreitz@redhat.com
Subject: Re: [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries
Date: Thu, 16 Jul 2020 12:39:17 +0300	[thread overview]
Message-ID: <8a020207-0a23-9407-a199-aea0af61170b@virtuozzo.com> (raw)
In-Reply-To: <1594676203-436999-8-git-send-email-andrey.shinkevich@virtuozzo.com>

14.07.2020 00:36, Andrey Shinkevich wrote:
> Add bitmap table information to the QCOW2 metadata dump.
> 
> Bitmap name               bitmap-1
> ...
> Bitmap table   type            offset                   size
> 0              serialized      4718592                  65536
> 1              serialized      4294967296               65536
> 2              serialized      5348033147437056         65536
> 3              serialized      13792273858822144        65536
> 4              serialized      4718592                  65536
> 5              serialized      4294967296               65536
> 6              serialized      4503608217305088         65536
> 7              serialized      14073748835532800        65536
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/qcow2_format.py | 42 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index d9c8513..2c78d46 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -175,14 +175,56 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>           entry_raw_size = self.bitmap_dir_entry_raw_size()
>           padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
>           fd.seek(padding, 1)
> +        position = fd.tell()
> +        self.read_bitmap_table(fd)
> +        fd.seek(position)
>   
>       def bitmap_dir_entry_raw_size(self):
>           return struct.calcsize(self.fmt) + self.name_size + \
>               self.extra_data_size
>   
> +    def read_bitmap_table(self, fd):
> +        fd.seek(self.bitmap_table_offset)
> +        table_size = self.bitmap_table_size * 8 * 8

s/* 8 * 8/* 8/ ?

> +        table = [e[0] for e in struct.iter_unpack('>Q', fd.read(table_size))]
> +        self.bitmap_table = Qcow2BitmapTable(raw_table=table,
> +                                             cluster_size=self.cluster_size)

Strange to read an unpack it here, when we have separate classes. It's obviously should be work of class: to read and parse its object data from file.

> +
>       def dump(self):
>           print(f'{"Bitmap name":<25} {self.name}')
>           super(Qcow2BitmapDirEntry, self).dump()
> +        self.bitmap_table.dump()
> +
> +
> +class Qcow2BitmapTableEntry:

Why not to derive it from Qcow2Struct? It will have only one field, but it will work, and we don't need to open-code loading in .read_bitmap_table of another class

> +
> +    BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffffffffffe00
> +    BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
> +
> +    def __init__(self, entry):
> +        self.offset = entry & self.BME_TABLE_ENTRY_OFFSET_MASK
> +        if self.offset:
> +            self.type = 'serialized'
> +        elif entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
> +            self.type = 'all-ones'
> +        else:
> +            self.type = 'all-zeroes'
> +
> +
> +class Qcow2BitmapTable:
> +
> +    def __init__(self, raw_table, cluster_size):

As well, probably no sense in deriving from Qcow2Struct, but for consistency, passing fd to __init__, and read all nested things here (list of Qcow2BitmapTableEntry) seems reasonable

> +        self.entries = []
> +        self.cluster_size = cluster_size
> +        for entry in raw_table:
> +            self.entries.append(Qcow2BitmapTableEntry(entry))
> +
> +    def dump(self):
> +        size = self.cluster_size
> +        bitmap_table = enumerate(self.entries)
> +        print(f'{"Bitmap table":<14} {"type":<15} {"offset":<24} {"size"}')
> +        for i, entry in bitmap_table:
> +            print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
>   
>   
>   QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
> 

-- 
Best regards,
Vladimir