Re: [PATCH v3 4/6] iotests: Dump bitmap directory info with qcow2.py
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 0xf bitmap_table_size 8 flag_bits 2 type 1 granularity_bits 16 name_size 8 extra_data_size 0 Suggested-by: Kevin Wolf Signed-off-by: Andrey Shinkevich --- 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: +
Re: [PATCH v3 4/6] iotests: Dump bitmap directory info with qcow2.py
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 0xf bitmap_table_size 8 flag_bits 2 type 1 granularity_bits 16 name_size 8 extra_data_size 0 Suggested-by: Kevin Wolf Signed-off-by: Andrey Shinkevich --- 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 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[PATCH 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 0xf bitmap_table_size 8 flag_bits 2 type 1 granularity_bits 16 name_size 8 extra_data_size 0 Suggested-by: Kevin Wolf Signed-off-by: Andrey Shinkevich --- 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'], +[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): + +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) + +self.bitmap_flags = [] +BME_FLAG_IN_USE = 1 +BME_FLAG_AUTO = 1 << 1 +if (self.flag_bits & BME_FLAG_IN_USE) != 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 + +def read_bitmap_directory(self, fd): +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 + + 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): -- 1.8.3.1