| Message ID | 1594676203-436999-7-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Dump QCOW2 dirty bitmaps metadata | expand |
14.07.2020 00:36, Andrey Shinkevich wrote: > The cluster size of an image is the QcowHeader class member and may be > obtained by dependent extension structures such as Qcow2BitmapExt for > further bitmap table details print. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > tests/qemu-iotests/qcow2_format.py | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py > index f0db9f4..d9c8513 100644 > --- a/tests/qemu-iotests/qcow2_format.py > +++ b/tests/qemu-iotests/qcow2_format.py > @@ -129,19 +129,21 @@ class Qcow2BitmapExt(Qcow2Struct): > ('u64', '{:#x}', 'bitmap_directory_offset') > ) > > - def __init__(self, fd): > + def __init__(self, fd, cluster_size): > super().__init__(fd=fd) > pad = (struct.calcsize(self.fmt) + 7) & ~7 > if pad: > fd.seek(pad, 1) > position = fd.tell() > + self.cluster_size = cluster_size > self.read_bitmap_directory(fd) > fd.seek(position) > > def read_bitmap_directory(self, fd): > fd.seek(self.bitmap_directory_offset) > self.bitmap_directory = \ > - [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)] > + [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size) > + for _ in range(self.nb_bitmaps)] Better to inline the bitmap directory loading code into __init__: Benefits: 1. Less code. read_bitmap_directory() is very small, used only in __init__ and just not needed as a separate method. __init__ is very small and simple too, so it's not a problem. 2. no need of extra self.cluster_size variable (you can use cluster_size parameter directly) 3. keep all fd.seek logic in one method but it's not about this patch. > > def dump(self): > super().dump() > @@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct): > ('u32', '{}', 'extra_data_size') > ) > > - def __init__(self, fd): > + def __init__(self, fd, cluster_size): > super().__init__(fd=fd) > + self.cluster_size = cluster_size > # Seek relative to the current position in the file > fd.seek(self.extra_data_size, 1) > bitmap_name = fd.read(self.name_size) > @@ -203,11 +206,13 @@ class QcowHeaderExtension(Qcow2Struct): > # then padding to next multiply of 8 > ) > > - def __init__(self, magic=None, length=None, data=None, fd=None): > + def __init__(self, magic=None, length=None, data=None, fd=None, > + cluster_size=None): > """ > Support both loading from fd and creation from user data. > For fd-based creation current position in a file will be used to read > the data. > + The cluster_size value may be obtained by dependent structures. > > This should be somehow refactored and functionality should be moved to > superclass (to allow creation of any qcow2 struct), but then, fields > @@ -230,7 +235,7 @@ class QcowHeaderExtension(Qcow2Struct): > assert all(v is None for v in (magic, length, data)) > super().__init__(fd=fd) > if self.magic == QCOW2_EXT_MAGIC_BITMAPS: > - self.obj = Qcow2BitmapExt(fd=fd) > + self.obj = Qcow2BitmapExt(fd=fd, cluster_size=cluster_size) > else: > padded = (self.length + 7) & ~7 > self.data = fd.read(padded) > @@ -244,7 +249,6 @@ class QcowHeaderExtension(Qcow2Struct): > data_str = '<binary>' > self.data_str = data_str > > - Unrelated style-fixing chunk, please don't do so. with it dropped: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > def dump(self): > super().dump() > > @@ -316,7 +320,7 @@ class QcowHeader(Qcow2Struct): > end = self.cluster_size > > while fd.tell() < end: > - ext = QcowHeaderExtension(fd=fd) > + ext = QcowHeaderExtension(fd=fd, cluster_size=self.cluster_size) > if ext.magic == 0: > break > else: >
On 16.07.2020 12:26, Vladimir Sementsov-Ogievskiy wrote: > 14.07.2020 00:36, Andrey Shinkevich wrote: >> The cluster size of an image is the QcowHeader class member and may be >> obtained by dependent extension structures such as Qcow2BitmapExt for >> further bitmap table details print. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> tests/qemu-iotests/qcow2_format.py | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/tests/qemu-iotests/qcow2_format.py >> b/tests/qemu-iotests/qcow2_format.py >> ... >> def read_bitmap_directory(self, fd): >> fd.seek(self.bitmap_directory_offset) >> self.bitmap_directory = \ >> - [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)] >> + [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size) >> + for _ in range(self.nb_bitmaps)] > > Better to inline the bitmap directory loading code into __init__: > > Benefits: > 1. Less code. read_bitmap_directory() is very small, used only in > __init__ and just not needed as a separate method. __init__ is very > small and simple too, so it's not a problem. > 2. no need of extra self.cluster_size variable (you can use > cluster_size parameter directly) > 3. keep all fd.seek logic in one method > > but it's not about this patch. > The idea behind the read_bitmap_directory() method was an encapsulation of reading the optional parameter. So, we can be flexible in future. Sure, there are prones and cons for that in the current implementation. Andrey >> def dump(self): >> super().dump() >> @@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct): >> ... >> @@ -244,7 +249,6 @@ class QcowHeaderExtension(Qcow2Struct): >> data_str = '<binary>' >> self.data_str = data_str >> - > > Unrelated style-fixing chunk, please don't do so. > > with it dropped: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> def dump(self): >> super().dump() >> @@ -316,7 +320,7 @@ class QcowHeader(Qcow2Struct): >> end = self.cluster_size >> while fd.tell() < end: >> - ext = QcowHeaderExtension(fd=fd) >> + ext = QcowHeaderExtension(fd=fd, >> cluster_size=self.cluster_size) >> if ext.magic == 0: >> break >> else: >> > >
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py index f0db9f4..d9c8513 100644 --- a/tests/qemu-iotests/qcow2_format.py +++ b/tests/qemu-iotests/qcow2_format.py @@ -129,19 +129,21 @@ class Qcow2BitmapExt(Qcow2Struct): ('u64', '{:#x}', 'bitmap_directory_offset') ) - def __init__(self, fd): + def __init__(self, fd, cluster_size): super().__init__(fd=fd) pad = (struct.calcsize(self.fmt) + 7) & ~7 if pad: fd.seek(pad, 1) position = fd.tell() + self.cluster_size = cluster_size self.read_bitmap_directory(fd) fd.seek(position) def read_bitmap_directory(self, fd): fd.seek(self.bitmap_directory_offset) self.bitmap_directory = \ - [Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)] + [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size) + for _ in range(self.nb_bitmaps)] def dump(self): super().dump() @@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct): ('u32', '{}', 'extra_data_size') ) - def __init__(self, fd): + def __init__(self, fd, cluster_size): super().__init__(fd=fd) + self.cluster_size = cluster_size # Seek relative to the current position in the file fd.seek(self.extra_data_size, 1) bitmap_name = fd.read(self.name_size) @@ -203,11 +206,13 @@ class QcowHeaderExtension(Qcow2Struct): # then padding to next multiply of 8 ) - def __init__(self, magic=None, length=None, data=None, fd=None): + def __init__(self, magic=None, length=None, data=None, fd=None, + cluster_size=None): """ Support both loading from fd and creation from user data. For fd-based creation current position in a file will be used to read the data. + The cluster_size value may be obtained by dependent structures. This should be somehow refactored and functionality should be moved to superclass (to allow creation of any qcow2 struct), but then, fields @@ -230,7 +235,7 @@ class QcowHeaderExtension(Qcow2Struct): assert all(v is None for v in (magic, length, data)) super().__init__(fd=fd) if self.magic == QCOW2_EXT_MAGIC_BITMAPS: - self.obj = Qcow2BitmapExt(fd=fd) + self.obj = Qcow2BitmapExt(fd=fd, cluster_size=cluster_size) else: padded = (self.length + 7) & ~7 self.data = fd.read(padded) @@ -244,7 +249,6 @@ class QcowHeaderExtension(Qcow2Struct): data_str = '<binary>' self.data_str = data_str - def dump(self): super().dump() @@ -316,7 +320,7 @@ class QcowHeader(Qcow2Struct): end = self.cluster_size while fd.tell() < end: - ext = QcowHeaderExtension(fd=fd) + ext = QcowHeaderExtension(fd=fd, cluster_size=self.cluster_size) if ext.magic == 0: break else:
The cluster size of an image is the QcowHeader class member and may be obtained by dependent extension structures such as Qcow2BitmapExt for further bitmap table details print. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- tests/qemu-iotests/qcow2_format.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)