* [PATCH v10 01/10] qcow2: Fix capitalization of header extension constant.
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-13 21:36 ` [PATCH v10 02/10] qcow2_format.py: make printable data an extension class member Andrey Shinkevich
` (8 subsequent siblings)
9 siblings, 0 replies; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
Make the capitalization of the hexadecimal numbers consistent for the
QCOW2 header extension constants in docs/interop/qcow2.txt.
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.c | 2 +-
docs/interop/qcow2.txt | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index ea33673..6c6bee3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -66,7 +66,7 @@ typedef struct {
} QEMU_PACKED QCowExtension;
#define QCOW2_EXT_MAGIC_END 0
-#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
+#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xe2792aca
#define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
#define QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
#define QCOW2_EXT_MAGIC_BITMAPS 0x23852875
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index cb72346..f072e27 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -231,7 +231,7 @@ be stored. Each extension has a structure like the following:
Byte 0 - 3: Header extension type:
0x00000000 - End of the header extension area
- 0xE2792ACA - Backing file format name string
+ 0xe2792aca - Backing file format name string
0x6803f857 - Feature name table
0x23852875 - Bitmaps extension
0x0537be77 - Full disk encryption header pointer
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v10 02/10] qcow2_format.py: make printable data an extension class member
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
2020-07-13 21:36 ` [PATCH v10 01/10] qcow2: Fix capitalization of header extension constant Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-13 21:36 ` [PATCH v10 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method Andrey Shinkevich
` (7 subsequent siblings)
9 siblings, 0 replies; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
Let us differ binary data type from string one for the extension data
variable and keep the string as the QcowHeaderExtension class member.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/qcow2_format.py | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index cc432e7..2f3681b 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -165,6 +165,13 @@ class QcowHeaderExtension(Qcow2Struct):
self.data = fd.read(padded)
assert self.data is not None
+ data_str = self.data[:self.length]
+ if all(c in string.printable.encode('ascii') for c in data_str):
+ data_str = f"'{ data_str.decode('ascii') }'"
+ else:
+ data_str = '<binary>'
+ self.data_str = data_str
+
if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
self.obj = Qcow2BitmapExt(data=self.data)
else:
@@ -174,12 +181,7 @@ class QcowHeaderExtension(Qcow2Struct):
super().dump()
if self.obj is None:
- data = self.data[:self.length]
- if all(c in string.printable.encode('ascii') for c in data):
- data = f"'{ data.decode('ascii') }'"
- else:
- data = '<binary>'
- print(f'{"data":<25} {data}')
+ print(f'{"data":<25} {self.data_str}')
else:
self.obj.dump()
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v10 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
2020-07-13 21:36 ` [PATCH v10 01/10] qcow2: Fix capitalization of header extension constant Andrey Shinkevich
2020-07-13 21:36 ` [PATCH v10 02/10] qcow2_format.py: make printable data an extension class member Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 8:47 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 04/10] qcow2_format.py: dump bitmap flags in human readable way Andrey Shinkevich
` (6 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/qcow2_format.py | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 2f3681b..cbaffc4 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -113,6 +113,11 @@ class Qcow2BitmapExt(Qcow2Struct):
('u64', '{:#x}', 'bitmap_directory_offset')
)
+ def __init__(self, fd):
+ super().__init__(fd=fd)
+ pad = (struct.calcsize(self.fmt) + 7) & ~7
+ if pad:
+ fd.seek(pad, 1)
QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
@@ -161,21 +166,21 @@ class QcowHeaderExtension(Qcow2Struct):
else:
assert all(v is None for v in (magic, length, data))
super().__init__(fd=fd)
- padded = (self.length + 7) & ~7
- self.data = fd.read(padded)
- assert self.data is not None
-
- data_str = self.data[:self.length]
- if all(c in string.printable.encode('ascii') for c in data_str):
- data_str = f"'{ data_str.decode('ascii') }'"
- else:
- data_str = '<binary>'
- self.data_str = data_str
+ if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
+ self.obj = Qcow2BitmapExt(fd=fd)
+ else:
+ padded = (self.length + 7) & ~7
+ self.data = fd.read(padded)
+ assert self.data is not None
+ self.obj = None
+ data_str = self.data[:self.length]
+ if all(c in string.printable.encode(
+ 'ascii') for c in data_str):
+ data_str = f"'{ data_str.decode('ascii') }'"
+ else:
+ data_str = '<binary>'
+ self.data_str = data_str
- if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
- self.obj = Qcow2BitmapExt(data=self.data)
- else:
- self.obj = None
def dump(self):
super().dump()
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method
2020-07-13 21:36 ` [PATCH v10 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method Andrey Shinkevich
@ 2020-07-16 8:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 8:47 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
14.07.2020 00:36, Andrey Shinkevich wrote:
> There are two ways to initialize a class derived from Qcow2Struct:
> 1. Pass a block of binary data to the constructor.
> 2. Pass the file descriptor to allow reading the file from constructor.
> Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
> support a scattered reading in the initialization chain.
> The implementation comes with the patch that follows.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> tests/qemu-iotests/qcow2_format.py | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index 2f3681b..cbaffc4 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -113,6 +113,11 @@ class Qcow2BitmapExt(Qcow2Struct):
> ('u64', '{:#x}', 'bitmap_directory_offset')
> )
>
> + def __init__(self, fd):
> + super().__init__(fd=fd)
> + pad = (struct.calcsize(self.fmt) + 7) & ~7
It's a size of structure rounded up to 8-bytes boundary. But after super init, we should alread be at the end of the structure and need to add only padding, not the whole structure rounded up. I think, correct code should be:
tail = struct.calcsize(self.fmt) % 8
if tail:
fd.seek(8 - tail, 1)
> + if pad:
> + fd.seek(pad, 1)
>
> QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
>
> @@ -161,21 +166,21 @@ class QcowHeaderExtension(Qcow2Struct):
> else:
Hmm, you parse data now only for this "else:" branch. Prior to this patch, it was parsed for "if fd is None:" branch as well, after the whole "if" statement. This is wrong.
> assert all(v is None for v in (magic, length, data))
> super().__init__(fd=fd)
> - padded = (self.length + 7) & ~7
> - self.data = fd.read(padded)
> - assert self.data is not None
> -
> - data_str = self.data[:self.length]
> - if all(c in string.printable.encode('ascii') for c in data_str):
> - data_str = f"'{ data_str.decode('ascii') }'"
> - else:
> - data_str = '<binary>'
> - self.data_str = data_str
> + if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
> + self.obj = Qcow2BitmapExt(fd=fd)
> + else:
> + padded = (self.length + 7) & ~7
> + self.data = fd.read(padded)
> + assert self.data is not None
> + self.obj = None
> + data_str = self.data[:self.length]
> + if all(c in string.printable.encode(
> + 'ascii') for c in data_str):
> + data_str = f"'{ data_str.decode('ascii') }'"
> + else:
> + data_str = '<binary>'
> + self.data_str = data_str
>
> - if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
> - self.obj = Qcow2BitmapExt(data=self.data)
> - else:
> - self.obj = None
>
> def dump(self):
> super().dump()
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v10 04/10] qcow2_format.py: dump bitmap flags in human readable way.
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (2 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 03/10] qcow2_format.py: change Qcow2BitmapExt initialization method Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 8:50 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 05/10] qcow2_format.py: Dump bitmap directory information Andrey Shinkevich
` (5 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
Introduce the class BitmapFlags that parses a bitmap flags mask.
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/qcow2_format.py | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index cbaffc4..e77c831 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -40,6 +40,22 @@ class Flags64(Qcow2Field):
return str(bits)
+class BitmapFlags(Qcow2Field):
+
+ flags = {
+ 0x1: 'in-use',
+ 0x2: 'auto'
+ }
+
+ def __str__(self):
+ bits = []
+ for bit in range(64):
+ flag = self.value & (1 << bit)
+ if flag:
+ bits.append(self.flags.get(flag, f'bit-{bit}'))
+ return f'{self.value:#x} ({bits})'
+
+
class Enum(Qcow2Field):
def __str__(self):
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v10 05/10] qcow2_format.py: Dump bitmap directory information
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (3 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 04/10] qcow2_format.py: dump bitmap flags in human readable way Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 9:14 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures Andrey Shinkevich
` (4 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
Read and dump entries from the bitmap directory of QCOW2 image.
Header extension:
magic 0x23852875 (Bitmaps)
...
Bitmap name bitmap-1
bitmap_table_offset 0xf0000
bitmap_table_size 1
flags 0x2 (['auto'])
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_format.py | 47 ++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index e77c831..f0db9f4 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -134,6 +134,53 @@ class Qcow2BitmapExt(Qcow2Struct):
pad = (struct.calcsize(self.fmt) + 7) & ~7
if pad:
fd.seek(pad, 1)
+ position = fd.tell()
+ 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)]
+
+ def dump(self):
+ super().dump()
+ for entry in self.bitmap_directory:
+ print()
+ entry.dump()
+
+
+class Qcow2BitmapDirEntry(Qcow2Struct):
+
+ fields = (
+ ('u64', '{:#x}', 'bitmap_table_offset'),
+ ('u32', '{}', 'bitmap_table_size'),
+ ('u32', BitmapFlags, 'flags'),
+ ('u8', '{}', 'type'),
+ ('u8', '{}', 'granularity_bits'),
+ ('u16', '{}', 'name_size'),
+ ('u32', '{}', 'extra_data_size')
+ )
+
+ def __init__(self, fd):
+ super().__init__(fd=fd)
+ # Seek relative to the current position in the file
+ fd.seek(self.extra_data_size, 1)
+ bitmap_name = fd.read(self.name_size)
+ self.name = bitmap_name.decode('ascii')
+ # Move position to the end of the entry in the directory
+ entry_raw_size = self.bitmap_dir_entry_raw_size()
+ padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
+ fd.seek(padding, 1)
+
+ def bitmap_dir_entry_raw_size(self):
+ return struct.calcsize(self.fmt) + self.name_size + \
+ self.extra_data_size
+
+ def dump(self):
+ print(f'{"Bitmap name":<25} {self.name}')
+ super(Qcow2BitmapDirEntry, self).dump()
+
QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 05/10] qcow2_format.py: Dump bitmap directory information
2020-07-13 21:36 ` [PATCH v10 05/10] qcow2_format.py: Dump bitmap directory information Andrey Shinkevich
@ 2020-07-16 9:14 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 9:14 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
14.07.2020 00:36, Andrey Shinkevich wrote:
> Read and dump entries from the bitmap directory of QCOW2 image.
>
> Header extension:
> magic 0x23852875 (Bitmaps)
> ...
> Bitmap name bitmap-1
> bitmap_table_offset 0xf0000
> bitmap_table_size 1
> flags 0x2 (['auto'])
> 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (4 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 05/10] qcow2_format.py: Dump bitmap directory information Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 9:26 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries Andrey Shinkevich
` (3 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
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)]
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:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures
2020-07-13 21:36 ` [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures Andrey Shinkevich
@ 2020-07-16 9:26 ` Vladimir Sementsov-Ogievskiy
2020-07-17 7:18 ` Andrey Shinkevich
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 9:26 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
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:
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures
2020-07-16 9:26 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-17 7:18 ` Andrey Shinkevich
0 siblings, 0 replies; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-17 7:18 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
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:
>>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (5 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 06/10] qcow2_format.py: pass cluster size to substructures Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 9:39 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format Andrey Shinkevich
` (2 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
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
+ 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)
+
def dump(self):
print(f'{"Bitmap name":<25} {self.name}')
super(Qcow2BitmapDirEntry, self).dump()
+ self.bitmap_table.dump()
+
+
+class Qcow2BitmapTableEntry:
+
+ 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):
+ 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
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries
2020-07-13 21:36 ` [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries Andrey Shinkevich
@ 2020-07-16 9:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 9:39 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (6 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 07/10] qcow2_format.py: Dump bitmap table serialized entries Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 10:20 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 09/10] qcow2_format.py: collect fields " Andrey Shinkevich
2020-07-13 21:36 ` [PATCH v10 10/10] qcow2_format.py: support dumping metadata " Andrey Shinkevich
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
Add the command key to the qcow2.py arguments list to dump QCOW2
metadata in JSON format. Here is the suggested way to do that. The
implementation of the dump in JSON format is in the patch that follows.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/qcow2.py | 19 +++++++++++++++----
tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 0910e6a..7402279 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -26,16 +26,19 @@ from qcow2_format import (
)
+dump_json = False
+
+
def cmd_dump_header(fd):
h = QcowHeader(fd)
- h.dump()
+ h.dump(dump_json)
print()
- h.dump_extensions()
+ h.dump_extensions(dump_json)
def cmd_dump_header_exts(fd):
h = QcowHeader(fd)
- h.dump_extensions()
+ h.dump_extensions(dump_json)
def cmd_set_header(fd, name, value):
@@ -134,6 +137,11 @@ cmds = [
def main(filename, cmd, args):
+ global dump_json
+ dump_json = '-j' in sys.argv
+ if dump_json:
+ sys.argv.remove('-j')
+ args.remove('-j')
fd = open(filename, "r+b")
try:
for name, handler, num_args, desc in cmds:
@@ -151,11 +159,14 @@ def main(filename, cmd, args):
def usage():
- print("Usage: %s <file> <cmd> [<arg>, ...]" % sys.argv[0])
+ print("Usage: %s <file> <cmd> [<arg>, ...] [<key>, ...]" % sys.argv[0])
print("")
print("Supported commands:")
for name, handler, num_args, desc in cmds:
print(" %-20s - %s" % (name, desc))
+ print("")
+ print("Supported keys:")
+ print(" %-20s - %s" % ('-j', 'Dump in JSON format'))
if __name__ == '__main__':
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 2c78d46..e0e14b5 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -109,7 +109,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
self.__dict__ = dict((field[2], values[i])
for i, field in enumerate(self.fields))
- def dump(self):
+ def dump(self, dump_json=None):
for f in self.fields:
value = self.__dict__[f[2]]
if isinstance(f[1], str):
@@ -145,8 +145,8 @@ class Qcow2BitmapExt(Qcow2Struct):
[Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
for _ in range(self.nb_bitmaps)]
- def dump(self):
- super().dump()
+ def dump(self, dump_json=None):
+ super().dump(dump_json)
for entry in self.bitmap_directory:
print()
entry.dump()
@@ -190,7 +190,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
self.bitmap_table = Qcow2BitmapTable(raw_table=table,
cluster_size=self.cluster_size)
- def dump(self):
+ def dump(self, dump_json=None):
print(f'{"Bitmap name":<25} {self.name}')
super(Qcow2BitmapDirEntry, self).dump()
self.bitmap_table.dump()
@@ -291,13 +291,13 @@ class QcowHeaderExtension(Qcow2Struct):
data_str = '<binary>'
self.data_str = data_str
- def dump(self):
+ def dump(self, dump_json=None):
super().dump()
if self.obj is None:
print(f'{"data":<25} {self.data_str}')
else:
- self.obj.dump()
+ self.obj.dump(dump_json)
@classmethod
def create(cls, magic, data):
@@ -396,8 +396,8 @@ class QcowHeader(Qcow2Struct):
buf = buf[0:header_bytes-1]
fd.write(buf)
- def dump_extensions(self):
+ def dump_extensions(self, dump_json=None):
for ex in self.extensions:
print('Header extension:')
- ex.dump()
+ ex.dump(dump_json)
print()
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format
2020-07-13 21:36 ` [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format Andrey Shinkevich
@ 2020-07-16 10:20 ` Vladimir Sementsov-Ogievskiy
2020-07-16 14:36 ` Andrey Shinkevich
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 10:20 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
14.07.2020 00:36, Andrey Shinkevich wrote:
> Add the command key to the qcow2.py arguments list to dump QCOW2
> metadata in JSON format. Here is the suggested way to do that. The
> implementation of the dump in JSON format is in the patch that follows.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> tests/qemu-iotests/qcow2.py | 19 +++++++++++++++----
> tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
> index 0910e6a..7402279 100755
> --- a/tests/qemu-iotests/qcow2.py
> +++ b/tests/qemu-iotests/qcow2.py
> @@ -26,16 +26,19 @@ from qcow2_format import (
> )
>
>
> +dump_json = False
> +
> +
> def cmd_dump_header(fd):
> h = QcowHeader(fd)
> - h.dump()
> + h.dump(dump_json)
> print()
> - h.dump_extensions()
> + h.dump_extensions(dump_json)
>
>
> def cmd_dump_header_exts(fd):
> h = QcowHeader(fd)
> - h.dump_extensions()
> + h.dump_extensions(dump_json)
>
>
> def cmd_set_header(fd, name, value):
> @@ -134,6 +137,11 @@ cmds = [
>
>
> def main(filename, cmd, args):
> + global dump_json
> + dump_json = '-j' in sys.argv
> + if dump_json:
> + sys.argv.remove('-j')
> + args.remove('-j')
I'd prefer to access sys.argv in one place (in "if __name__ ...").
> fd = open(filename, "r+b")
> try:
> for name, handler, num_args, desc in cmds:
> @@ -151,11 +159,14 @@ def main(filename, cmd, args):
>
>
> def usage():
> - print("Usage: %s <file> <cmd> [<arg>, ...]" % sys.argv[0])
> + print("Usage: %s <file> <cmd> [<arg>, ...] [<key>, ...]" % sys.argv[0])
> print("")
> print("Supported commands:")
> for name, handler, num_args, desc in cmds:
> print(" %-20s - %s" % (name, desc))
> + print("")
> + print("Supported keys:")
> + print(" %-20s - %s" % ('-j', 'Dump in JSON format'))
>
>
> if __name__ == '__main__':
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index 2c78d46..e0e14b5 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -109,7 +109,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
> self.__dict__ = dict((field[2], values[i])
> for i, field in enumerate(self.fields))
>
> - def dump(self):
> + def dump(self, dump_json=None):
> for f in self.fields:
> value = self.__dict__[f[2]]
> if isinstance(f[1], str):
> @@ -145,8 +145,8 @@ class Qcow2BitmapExt(Qcow2Struct):
> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
> for _ in range(self.nb_bitmaps)]
>
> - def dump(self):
> - super().dump()
> + def dump(self, dump_json=None):
strange to make None the default value for boolean. Why not just False?
Also, why not call it just "json"? We are already in "dump" context, no needs to add prefix to the name.
> + super().dump(dump_json)
> for entry in self.bitmap_directory:
> print()
> entry.dump()
How will it work? You are interleaving json dump and non-json?
Looking at this, I think that json and non-json dumps has more differences than similarities, and it probably simpler to make a separate function dump_json.. But I'm absolutely against an option, if it will be done consistently.
> @@ -190,7 +190,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
> self.bitmap_table = Qcow2BitmapTable(raw_table=table,
> cluster_size=self.cluster_size)
>
> - def dump(self):
> + def dump(self, dump_json=None):
> print(f'{"Bitmap name":<25} {self.name}')
> super(Qcow2BitmapDirEntry, self).dump()
> self.bitmap_table.dump()
Here the new option just not passed to the nested dump() calls..
> @@ -291,13 +291,13 @@ class QcowHeaderExtension(Qcow2Struct):
> data_str = '<binary>'
> self.data_str = data_str
>
> - def dump(self):
> + def dump(self, dump_json=None):
> super().dump()
>
> if self.obj is None:
> print(f'{"data":<25} {self.data_str}')
> else:
> - self.obj.dump()
> + self.obj.dump(dump_json)
>
> @classmethod
> def create(cls, magic, data):
> @@ -396,8 +396,8 @@ class QcowHeader(Qcow2Struct):
> buf = buf[0:header_bytes-1]
> fd.write(buf)
>
> - def dump_extensions(self):
> + def dump_extensions(self, dump_json=None):
> for ex in self.extensions:
> print('Header extension:')
> - ex.dump()
> + ex.dump(dump_json)
> print()
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format
2020-07-16 10:20 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-16 14:36 ` Andrey Shinkevich
2020-07-16 14:38 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-16 14:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
On 16.07.2020 13:20, Vladimir Sementsov-Ogievskiy wrote:
> 14.07.2020 00:36, Andrey Shinkevich wrote:
>> Add the command key to the qcow2.py arguments list to dump QCOW2
>> metadata in JSON format. Here is the suggested way to do that. The
>> implementation of the dump in JSON format is in the patch that follows.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> tests/qemu-iotests/qcow2.py | 19 +++++++++++++++----
>> tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
>> 2 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
>> index 0910e6a..7402279 100755
...
>> @@ -134,6 +137,11 @@ cmds = [
>> def main(filename, cmd, args):
>> + global dump_json
>> + dump_json = '-j' in sys.argv
>> + if dump_json:
>> + sys.argv.remove('-j')
>> + args.remove('-j')
>
> I'd prefer to access sys.argv in one place (in "if __name__ ...").
>
In case we move the block
global dump_json
dump_json = '-j' in sys.argv
if dump_json:
sys.argv.remove('-j')
from main() to the body of "if __name__ == '__main__'"
we get the following error -
"SyntaxError: name 'dump_json' is assigned to before global declaration"
Andrey
>> fd = open(filename, "r+b")
>> try:
>> for name, handler, num_args, desc in cmds:
>> @@ -151,11 +159,14 @@ def main(filename, cmd, args):
>> def usage():
>> - print("Usage: %s <file> <cmd> [<arg>, ...]" % sys.argv[0])
>> + print("Usage: %s <file> <cmd> [<arg>, ...] [<key>, ...]" %
>> sys.argv[0])
>> print("")
>> print("Supported commands:")
>> for name, handler, num_args, desc in cmds:
>> print(" %-20s - %s" % (name, desc))
>> + print("")
>> + print("Supported keys:")
>> + print(" %-20s - %s" % ('-j', 'Dump in JSON format'))
>> if __name__ == '__main__':
>> diff --git a/tests/qemu-iotests/qcow2_format.py
>> b/tests/qemu-iotests/qcow2_format.py
>> index 2c78d46..e0e14b5 100644
>> --- a/tests/qemu-iotests/qcow2_format.py
>> +++ b/tests/qemu-iotests/qcow2_format.py
>> @@ -109,7 +109,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>> self.__dict__ = dict((field[2], values[i])
>> for i, field in enumerate(self.fields))
>> - def dump(self):
>> + def dump(self, dump_json=None):
>> for f in self.fields:
>> value = self.__dict__[f[2]]
>> if isinstance(f[1], str):
>> @@ -145,8 +145,8 @@ class Qcow2BitmapExt(Qcow2Struct):
>> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>> for _ in range(self.nb_bitmaps)]
>> - def dump(self):
>> - super().dump()
>> + def dump(self, dump_json=None):
>
> strange to make None the default value for boolean. Why not just False?
>
> Also, why not call it just "json"? We are already in "dump" context,
> no needs to add prefix to the name.
>
>> + super().dump(dump_json)
>> for entry in self.bitmap_directory:
>> print()
>> entry.dump()
>
> How will it work? You are interleaving json dump and non-json?
>
> Looking at this, I think that json and non-json dumps has more
> differences than similarities, and it probably simpler to make a
> separate function dump_json.. But I'm absolutely against an option, if
> it will be done consistently.
>
>> @@ -190,7 +190,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>> self.bitmap_table = Qcow2BitmapTable(raw_table=table,
>> cluster_size=self.cluster_size)
>> - def dump(self):
>> + def dump(self, dump_json=None):
>> print(f'{"Bitmap name":<25} {self.name}')
>> super(Qcow2BitmapDirEntry, self).dump()
>> self.bitmap_table.dump()
>
> Here the new option just not passed to the nested dump() calls..
>
>> @@ -291,13 +291,13 @@ class QcowHeaderExtension(Qcow2Struct):
>> data_str = '<binary>'
>> self.data_str = data_str
>> - def dump(self):
>> + def dump(self, dump_json=None):
>> super().dump()
>> if self.obj is None:
>> print(f'{"data":<25} {self.data_str}')
>> else:
>> - self.obj.dump()
>> + self.obj.dump(dump_json)
>> @classmethod
>> def create(cls, magic, data):
>> @@ -396,8 +396,8 @@ class QcowHeader(Qcow2Struct):
>> buf = buf[0:header_bytes-1]
>> fd.write(buf)
>> - def dump_extensions(self):
>> + def dump_extensions(self, dump_json=None):
>> for ex in self.extensions:
>> print('Header extension:')
>> - ex.dump()
>> + ex.dump(dump_json)
>> print()
>>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format
2020-07-16 14:36 ` Andrey Shinkevich
@ 2020-07-16 14:38 ` Vladimir Sementsov-Ogievskiy
2020-07-16 14:44 ` Andrey Shinkevich
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 14:38 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
16.07.2020 17:36, Andrey Shinkevich wrote:
> On 16.07.2020 13:20, Vladimir Sementsov-Ogievskiy wrote:
>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>> Add the command key to the qcow2.py arguments list to dump QCOW2
>>> metadata in JSON format. Here is the suggested way to do that. The
>>> implementation of the dump in JSON format is in the patch that follows.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>> tests/qemu-iotests/qcow2.py | 19 +++++++++++++++----
>>> tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
>>> 2 files changed, 23 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
>>> index 0910e6a..7402279 100755
> ...
>>> @@ -134,6 +137,11 @@ cmds = [
>>> def main(filename, cmd, args):
>>> + global dump_json
>>> + dump_json = '-j' in sys.argv
>>> + if dump_json:
>>> + sys.argv.remove('-j')
>>> + args.remove('-j')
>>
>> I'd prefer to access sys.argv in one place (in "if __name__ ...").
>>
>
> In case we move the block
>
> global dump_json
> dump_json = '-j' in sys.argv
>
> if dump_json:
> sys.argv.remove('-j')
>
> from main() to the body of "if __name__ == '__main__'"
>
> we get the following error -
>
> "SyntaxError: name 'dump_json' is assigned to before global declaration"
you don't need "global" declaration outside of a function
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format
2020-07-16 14:38 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-16 14:44 ` Andrey Shinkevich
2020-07-16 14:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-16 14:44 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
On 16.07.2020 17:38, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 17:36, Andrey Shinkevich wrote:
>> On 16.07.2020 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>>> Add the command key to the qcow2.py arguments list to dump QCOW2
>>>> metadata in JSON format. Here is the suggested way to do that. The
>>>> implementation of the dump in JSON format is in the patch that
>>>> follows.
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>> tests/qemu-iotests/qcow2.py | 19 +++++++++++++++----
>>>> tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
>>>> 2 files changed, 23 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
>>>> index 0910e6a..7402279 100755
>> ...
>>>> @@ -134,6 +137,11 @@ cmds = [
>>>> def main(filename, cmd, args):
>>>> + global dump_json
>>>> + dump_json = '-j' in sys.argv
>>>> + if dump_json:
>>>> + sys.argv.remove('-j')
>>>> + args.remove('-j')
>>>
>>> I'd prefer to access sys.argv in one place (in "if __name__ ...").
>>>
>>
>> In case we move the block
>>
>> global dump_json
>> dump_json = '-j' in sys.argv
>>
>> if dump_json:
>> sys.argv.remove('-j')
>>
>> from main() to the body of "if __name__ == '__main__'"
>>
>> we get the following error -
>>
>> "SyntaxError: name 'dump_json' is assigned to before global declaration"
>
> you don't need "global" declaration outside of a function
>
>
OK, thanks. However, if we want to parse more keys in future, will we do
all that parsing in the body of "if __name__ == '__main__'"?
Andrey
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format
2020-07-16 14:44 ` Andrey Shinkevich
@ 2020-07-16 14:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 14:47 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
16.07.2020 17:44, Andrey Shinkevich wrote:
> On 16.07.2020 17:38, Vladimir Sementsov-Ogievskiy wrote:
>> 16.07.2020 17:36, Andrey Shinkevich wrote:
>>> On 16.07.2020 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>>>> Add the command key to the qcow2.py arguments list to dump QCOW2
>>>>> metadata in JSON format. Here is the suggested way to do that. The
>>>>> implementation of the dump in JSON format is in the patch that follows.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>> tests/qemu-iotests/qcow2.py | 19 +++++++++++++++----
>>>>> tests/qemu-iotests/qcow2_format.py | 16 ++++++++--------
>>>>> 2 files changed, 23 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
>>>>> index 0910e6a..7402279 100755
>>> ...
>>>>> @@ -134,6 +137,11 @@ cmds = [
>>>>> def main(filename, cmd, args):
>>>>> + global dump_json
>>>>> + dump_json = '-j' in sys.argv
>>>>> + if dump_json:
>>>>> + sys.argv.remove('-j')
>>>>> + args.remove('-j')
>>>>
>>>> I'd prefer to access sys.argv in one place (in "if __name__ ...").
>>>>
>>>
>>> In case we move the block
>>>
>>> global dump_json
>>> dump_json = '-j' in sys.argv
>>>
>>> if dump_json:
>>> sys.argv.remove('-j')
>>>
>>> from main() to the body of "if __name__ == '__main__'"
>>>
>>> we get the following error -
>>>
>>> "SyntaxError: name 'dump_json' is assigned to before global declaration"
>>
>> you don't need "global" declaration outside of a function
>>
>>
>
> OK, thanks. However, if we want to parse more keys in future, will we do all that parsing in the body of "if __name__ == '__main__'"?
>
If we are going to parse more key, we should move to ArgumentParser first. It may be in body of "if" or in a function..
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (7 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 08/10] qcow2.py: Introduce '-j' key to dump in JSON format Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 10:24 ` Vladimir Sementsov-Ogievskiy
2020-07-13 21:36 ` [PATCH v10 10/10] qcow2_format.py: support dumping metadata " Andrey Shinkevich
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
As __dict__ is being extended with class members we do not want to
print, make a light copy of the initial __dict__ and extend the copy
by adding lists we have to print in the JSON output.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/qcow2_format.py | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index e0e14b5..83c3482 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
self.__dict__ = dict((field[2], values[i])
for i, field in enumerate(self.fields))
+ self.fields_dict = self.__dict__.copy()
+
def dump(self, dump_json=None):
for f in self.fields:
value = self.__dict__[f[2]]
@@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
self.bitmap_directory = \
[Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
for _ in range(self.nb_bitmaps)]
+ self.fields_dict.update(bitmap_directory=self.bitmap_directory)
def dump(self, dump_json=None):
super().dump(dump_json)
@@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
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)
+ self.fields_dict.update(bitmap_table=self.bitmap_table)
def dump(self, dump_json=None):
print(f'{"Bitmap name":<25} {self.name}')
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
2020-07-13 21:36 ` [PATCH v10 09/10] qcow2_format.py: collect fields " Andrey Shinkevich
@ 2020-07-16 10:24 ` Vladimir Sementsov-Ogievskiy
2020-07-16 15:34 ` Andrey Shinkevich
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 10:24 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
14.07.2020 00:36, Andrey Shinkevich wrote:
> As __dict__ is being extended with class members we do not want to
> print, make a light copy of the initial __dict__ and extend the copy
> by adding lists we have to print in the JSON output.
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> tests/qemu-iotests/qcow2_format.py | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index e0e14b5..83c3482 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
> self.__dict__ = dict((field[2], values[i])
> for i, field in enumerate(self.fields))
>
> + self.fields_dict = self.__dict__.copy()
No, I don't like that. Keeping two copies of all the data is bad idea. If you want to select some fields, add a method (dump_json() ?) Which will collect the fields you want in a dict and return that new dict. But don't store object attributes twice.
> +
> def dump(self, dump_json=None):
> for f in self.fields:
> value = self.__dict__[f[2]]
> @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
> self.bitmap_directory = \
> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
> for _ in range(self.nb_bitmaps)]
> + self.fields_dict.update(bitmap_directory=self.bitmap_directory)
>
> def dump(self, dump_json=None):
> super().dump(dump_json)
> @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
> 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)
> + self.fields_dict.update(bitmap_table=self.bitmap_table)
>
> def dump(self, dump_json=None):
> print(f'{"Bitmap name":<25} {self.name}')
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
2020-07-16 10:24 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-16 15:34 ` Andrey Shinkevich
2020-07-16 15:40 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-16 15:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
On 16.07.2020 13:24, Vladimir Sementsov-Ogievskiy wrote:
> 14.07.2020 00:36, Andrey Shinkevich wrote:
>> As __dict__ is being extended with class members we do not want to
>> print, make a light copy of the initial __dict__ and extend the copy
>> by adding lists we have to print in the JSON output.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> tests/qemu-iotests/qcow2_format.py | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/qcow2_format.py
>> b/tests/qemu-iotests/qcow2_format.py
>> index e0e14b5..83c3482 100644
>> --- a/tests/qemu-iotests/qcow2_format.py
>> +++ b/tests/qemu-iotests/qcow2_format.py
>> @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>> self.__dict__ = dict((field[2], values[i])
>> for i, field in enumerate(self.fields))
>> + self.fields_dict = self.__dict__.copy()
>
> No, I don't like that. Keeping two copies of all the data is bad idea.
> If you want to select some fields, add a method (dump_json() ?) Which
> will collect the fields you want in a dict and return that new dict.
> But don't store object attributes twice.
>
Not really. It makes a light copy that stores only references to the
desired fields.
Andrey
>> +
>> def dump(self, dump_json=None):
>> for f in self.fields:
>> value = self.__dict__[f[2]]
>> @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
>> self.bitmap_directory = \
>> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>> for _ in range(self.nb_bitmaps)]
>> + self.fields_dict.update(bitmap_directory=self.bitmap_directory)
>> def dump(self, dump_json=None):
>> super().dump(dump_json)
>> @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>> 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)
>> + self.fields_dict.update(bitmap_table=self.bitmap_table)
>> def dump(self, dump_json=None):
>> print(f'{"Bitmap name":<25} {self.name}')
>>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
2020-07-16 15:34 ` Andrey Shinkevich
@ 2020-07-16 15:40 ` Vladimir Sementsov-Ogievskiy
2020-07-16 15:52 ` Andrey Shinkevich
0 siblings, 1 reply; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 15:40 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
16.07.2020 18:34, Andrey Shinkevich wrote:
> On 16.07.2020 13:24, Vladimir Sementsov-Ogievskiy wrote:
>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>> As __dict__ is being extended with class members we do not want to
>>> print, make a light copy of the initial __dict__ and extend the copy
>>> by adding lists we have to print in the JSON output.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>> tests/qemu-iotests/qcow2_format.py | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
>>> index e0e14b5..83c3482 100644
>>> --- a/tests/qemu-iotests/qcow2_format.py
>>> +++ b/tests/qemu-iotests/qcow2_format.py
>>> @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>>> self.__dict__ = dict((field[2], values[i])
>>> for i, field in enumerate(self.fields))
>>> + self.fields_dict = self.__dict__.copy()
>>
>> No, I don't like that. Keeping two copies of all the data is bad idea. If you want to select some fields, add a method (dump_json() ?) Which will collect the fields you want in a dict and return that new dict. But don't store object attributes twice.
>>
>
> Not really. It makes a light copy that stores only references to the desired fields.
>
I'm not about memory consumption, I'm about the design. Keeping two representations of same thing is always painful to maintain.
>
>>> +
>>> def dump(self, dump_json=None):
>>> for f in self.fields:
>>> value = self.__dict__[f[2]]
>>> @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
>>> self.bitmap_directory = \
>>> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>>> for _ in range(self.nb_bitmaps)]
>>> + self.fields_dict.update(bitmap_directory=self.bitmap_directory)
>>> def dump(self, dump_json=None):
>>> super().dump(dump_json)
>>> @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>>> 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)
>>> + self.fields_dict.update(bitmap_table=self.bitmap_table)
>>> def dump(self, dump_json=None):
>>> print(f'{"Bitmap name":<25} {self.name}')
>>>
>>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
2020-07-16 15:40 ` Vladimir Sementsov-Ogievskiy
@ 2020-07-16 15:52 ` Andrey Shinkevich
2020-07-16 16:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-16 15:52 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
On 16.07.2020 18:40, Vladimir Sementsov-Ogievskiy wrote:
> 16.07.2020 18:34, Andrey Shinkevich wrote:
>> On 16.07.2020 13:24, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>>> As __dict__ is being extended with class members we do not want to
>>>> print, make a light copy of the initial __dict__ and extend the copy
>>>> by adding lists we have to print in the JSON output.
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>> tests/qemu-iotests/qcow2_format.py | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/tests/qemu-iotests/qcow2_format.py
>>>> b/tests/qemu-iotests/qcow2_format.py
>>>> index e0e14b5..83c3482 100644
>>>> --- a/tests/qemu-iotests/qcow2_format.py
>>>> +++ b/tests/qemu-iotests/qcow2_format.py
>>>> @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>>>> self.__dict__ = dict((field[2], values[i])
>>>> for i, field in enumerate(self.fields))
>>>> + self.fields_dict = self.__dict__.copy()
>>>
>>> No, I don't like that. Keeping two copies of all the data is bad
>>> idea. If you want to select some fields, add a method (dump_json()
>>> ?) Which will collect the fields you want in a dict and return that
>>> new dict. But don't store object attributes twice.
That is what I did in the versions before but it looks clumsy to me.
Every single class lists almost all the items of the __dict__ again in
the additional method.
Andrey
>>>
>>
>> Not really. It makes a light copy that stores only references to the
>> desired fields.
>>
>
> I'm not about memory consumption, I'm about the design. Keeping two
> representations of same thing is always painful to maintain.
>
>>
>>>> +
>>>> def dump(self, dump_json=None):
>>>> for f in self.fields:
>>>> value = self.__dict__[f[2]]
>>>> @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
>>>> self.bitmap_directory = \
>>>> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>>>> for _ in range(self.nb_bitmaps)]
>>>> + self.fields_dict.update(bitmap_directory=self.bitmap_directory)
>>>> def dump(self, dump_json=None):
>>>> super().dump(dump_json)
>>>> @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>>>> 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)
>>>> + self.fields_dict.update(bitmap_table=self.bitmap_table)
>>>> def dump(self, dump_json=None):
>>>> print(f'{"Bitmap name":<25} {self.name}')
>>>>
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v10 09/10] qcow2_format.py: collect fields to dump in JSON format
2020-07-16 15:52 ` Andrey Shinkevich
@ 2020-07-16 16:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 16:08 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
16.07.2020 18:52, Andrey Shinkevich wrote:
> On 16.07.2020 18:40, Vladimir Sementsov-Ogievskiy wrote:
>> 16.07.2020 18:34, Andrey Shinkevich wrote:
>>> On 16.07.2020 13:24, Vladimir Sementsov-Ogievskiy wrote:
>>>> 14.07.2020 00:36, Andrey Shinkevich wrote:
>>>>> As __dict__ is being extended with class members we do not want to
>>>>> print, make a light copy of the initial __dict__ and extend the copy
>>>>> by adding lists we have to print in the JSON output.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> ---
>>>>> tests/qemu-iotests/qcow2_format.py | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
>>>>> index e0e14b5..83c3482 100644
>>>>> --- a/tests/qemu-iotests/qcow2_format.py
>>>>> +++ b/tests/qemu-iotests/qcow2_format.py
>>>>> @@ -109,6 +109,8 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
>>>>> self.__dict__ = dict((field[2], values[i])
>>>>> for i, field in enumerate(self.fields))
>>>>> + self.fields_dict = self.__dict__.copy()
>>>>
>>>> No, I don't like that. Keeping two copies of all the data is bad idea. If you want to select some fields, add a method (dump_json() ?) Which will collect the fields you want in a dict and return that new dict. But don't store object attributes twice.
>
>
> That is what I did in the versions before but it looks clumsy to me. Every single class lists almost all the items of the __dict__ again in the additional method.
>
Most of them should be listed automatically by base class method, which will iterate through .fields
>
>>>>
>>>
>>> Not really. It makes a light copy that stores only references to the desired fields.
>>>
>>
>> I'm not about memory consumption, I'm about the design. Keeping two representations of same thing is always painful to maintain.
>>
>>>
>>>>> +
>>>>> def dump(self, dump_json=None):
>>>>> for f in self.fields:
>>>>> value = self.__dict__[f[2]]
>>>>> @@ -144,6 +146,7 @@ class Qcow2BitmapExt(Qcow2Struct):
>>>>> self.bitmap_directory = \
>>>>> [Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
>>>>> for _ in range(self.nb_bitmaps)]
>>>>> + self.fields_dict.update(bitmap_directory=self.bitmap_directory)
>>>>> def dump(self, dump_json=None):
>>>>> super().dump(dump_json)
>>>>> @@ -189,6 +192,7 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
>>>>> 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)
>>>>> + self.fields_dict.update(bitmap_table=self.bitmap_table)
>>>>> def dump(self, dump_json=None):
>>>>> print(f'{"Bitmap name":<25} {self.name}')
>>>>>
>>>>
>>>>
>>
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v10 10/10] qcow2_format.py: support dumping metadata in JSON format
2020-07-13 21:36 [PATCH v10 00/10] iotests: Dump QCOW2 dirty bitmaps metadata Andrey Shinkevich
` (8 preceding siblings ...)
2020-07-13 21:36 ` [PATCH v10 09/10] qcow2_format.py: collect fields " Andrey Shinkevich
@ 2020-07-13 21:36 ` Andrey Shinkevich
2020-07-16 10:37 ` Vladimir Sementsov-Ogievskiy
9 siblings, 1 reply; 28+ messages in thread
From: Andrey Shinkevich @ 2020-07-13 21:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz, andrey.shinkevich, den
Implementation of dumping QCOW2 image metadata.
The sample output:
{
"Header_extensions": [
{
"name": "Feature table",
"magic": 1745090647,
"length": 192,
"data_str": "<binary>"
},
{
"name": "Bitmaps",
"magic": 595929205,
"length": 24,
"data": {
"nb_bitmaps": 2,
"reserved32": 0,
"bitmap_directory_size": 64,
"bitmap_directory_offset": 1048576,
"bitmap_directory": [
{
"name": "bitmap-1",
"bitmap_table_offset": 589824,
"bitmap_table_size": 1,
"flags": 2,
"type": 1,
"granularity_bits": 15,
"name_size": 8,
"extra_data_size": 0,
"bitmap_table": {
"entries": [
{
"type": "serialized",
"offset": 655360
},
...
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
tests/qemu-iotests/qcow2_format.py | 59 ++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
index 83c3482..a263858 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -19,6 +19,15 @@
import struct
import string
+import json
+
+
+class ComplexEncoder(json.JSONEncoder):
+ def default(self, obj):
+ if hasattr(obj, 'get_fields_dict'):
+ return obj.get_fields_dict()
+ else:
+ return json.JSONEncoder.default(self, obj)
class Qcow2Field:
@@ -112,6 +121,11 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
self.fields_dict = self.__dict__.copy()
def dump(self, dump_json=None):
+ if dump_json:
+ print(json.dumps(self.get_fields_dict(), indent=4,
+ cls=ComplexEncoder))
+ return
+
for f in self.fields:
value = self.__dict__[f[2]]
if isinstance(f[1], str):
@@ -154,6 +168,9 @@ class Qcow2BitmapExt(Qcow2Struct):
print()
entry.dump()
+ def get_fields_dict(self):
+ return self.fields_dict
+
class Qcow2BitmapDirEntry(Qcow2Struct):
@@ -199,6 +216,11 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
super(Qcow2BitmapDirEntry, self).dump()
self.bitmap_table.dump()
+ def get_fields_dict(self):
+ bmp_name = dict(name=self.name)
+ bme_dict = {**bmp_name, **self.fields_dict}
+ return bme_dict
+
class Qcow2BitmapTableEntry:
@@ -214,6 +236,9 @@ class Qcow2BitmapTableEntry:
else:
self.type = 'all-zeroes'
+ def get_fields_dict(self):
+ return dict(type=self.type, offset=self.offset)
+
class Qcow2BitmapTable:
@@ -230,6 +255,18 @@ class Qcow2BitmapTable:
for i, entry in bitmap_table:
print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
+ def get_fields_dict(self):
+ return dict(entries=self.entries)
+
+
+class Qcow2HeaderExtensionsDoc:
+
+ def __init__(self, extensions):
+ self.extensions = extensions
+
+ def get_fields_dict(self):
+ return dict(Header_extensions=self.extensions)
+
QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
@@ -245,6 +282,9 @@ class QcowHeaderExtension(Qcow2Struct):
0x44415441: 'Data file'
}
+ def get_fields_dict(self):
+ return self.mapping.get(self.value, "<unknown>")
+
fields = (
('u32', Magic, 'magic'),
('u32', '{}', 'length')
@@ -303,6 +343,16 @@ class QcowHeaderExtension(Qcow2Struct):
else:
self.obj.dump(dump_json)
+ def get_fields_dict(self):
+ ext_name = dict(name=self.Magic(self.magic))
+ he_dict = {**ext_name, **self.fields_dict}
+ if self.obj is not None:
+ he_dict.update(data=self.obj)
+ else:
+ he_dict.update(data_str=self.data_str)
+
+ return he_dict
+
@classmethod
def create(cls, magic, data):
return QcowHeaderExtension(magic, len(data), data)
@@ -401,7 +451,16 @@ class QcowHeader(Qcow2Struct):
fd.write(buf)
def dump_extensions(self, dump_json=None):
+ if dump_json:
+ ext_doc = Qcow2HeaderExtensionsDoc(self.extensions)
+ print(json.dumps(ext_doc.get_fields_dict(), indent=4,
+ cls=ComplexEncoder))
+ return
+
for ex in self.extensions:
print('Header extension:')
ex.dump(dump_json)
print()
+
+ def get_fields_dict(self):
+ return self.fields_dict
--
1.8.3.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v10 10/10] qcow2_format.py: support dumping metadata in JSON format
2020-07-13 21:36 ` [PATCH v10 10/10] qcow2_format.py: support dumping metadata " Andrey Shinkevich
@ 2020-07-16 10:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 28+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-07-16 10:37 UTC (permalink / raw)
To: Andrey Shinkevich, qemu-block; +Cc: kwolf, den, qemu-devel, mreitz
14.07.2020 00:36, Andrey Shinkevich wrote:
> Implementation of dumping QCOW2 image metadata.
> The sample output:
> {
> "Header_extensions": [
> {
> "name": "Feature table",
> "magic": 1745090647,
> "length": 192,
> "data_str": "<binary>"
> },
> {
> "name": "Bitmaps",
> "magic": 595929205,
> "length": 24,
> "data": {
> "nb_bitmaps": 2,
> "reserved32": 0,
> "bitmap_directory_size": 64,
> "bitmap_directory_offset": 1048576,
> "bitmap_directory": [
> {
> "name": "bitmap-1",
> "bitmap_table_offset": 589824,
> "bitmap_table_size": 1,
> "flags": 2,
> "type": 1,
> "granularity_bits": 15,
> "name_size": 8,
> "extra_data_size": 0,
> "bitmap_table": {
> "entries": [
> {
> "type": "serialized",
> "offset": 655360
> },
> ...
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Aha, so you actually do what I imagine in a previous patch, add a method returning dict (I'd call it just "to_dict", but that doesn't really matter). (Stil, I'm against keeping self.fields_dict attribute. Just make correct .to_dict() method for base class (which will utilize .fields), and call super().to_dict() and add some another fields in children classes (like it is done for dump() method)
> ---
> tests/qemu-iotests/qcow2_format.py | 59 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
> diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py
> index 83c3482..a263858 100644
> --- a/tests/qemu-iotests/qcow2_format.py
> +++ b/tests/qemu-iotests/qcow2_format.py
> @@ -19,6 +19,15 @@
>
> import struct
> import string
> +import json
> +
> +
> +class ComplexEncoder(json.JSONEncoder):
> + def default(self, obj):
> + if hasattr(obj, 'get_fields_dict'):
> + return obj.get_fields_dict()
> + else:
> + return json.JSONEncoder.default(self, obj)
>
>
> class Qcow2Field:
> @@ -112,6 +121,11 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
> self.fields_dict = self.__dict__.copy()
>
> def dump(self, dump_json=None):
> + if dump_json:
> + print(json.dumps(self.get_fields_dict(), indent=4,
> + cls=ComplexEncoder))
> + return
Now it is obvious, that this is enough: we can just make it a seprate method dump_json in Qcow2Struct and we'll never need to implement dump_json in other classes (only .to_dict). But really, with dump_json parameter other classes are wrong now: they call super().dump(dump_json) which dumps json, and they they print additional things in a non-json format.
> +
> for f in self.fields:
> value = self.__dict__[f[2]]
> if isinstance(f[1], str):
> @@ -154,6 +168,9 @@ class Qcow2BitmapExt(Qcow2Struct):
> print()
> entry.dump()
>
> + def get_fields_dict(self):
> + return self.fields_dict
> +
>
> class Qcow2BitmapDirEntry(Qcow2Struct):
>
> @@ -199,6 +216,11 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
> super(Qcow2BitmapDirEntry, self).dump()
> self.bitmap_table.dump()
>
> + def get_fields_dict(self):
> + bmp_name = dict(name=self.name)
> + bme_dict = {**bmp_name, **self.fields_dict}
> + return bme_dict
> +
>
> class Qcow2BitmapTableEntry:
>
> @@ -214,6 +236,9 @@ class Qcow2BitmapTableEntry:
> else:
> self.type = 'all-zeroes'
>
> + def get_fields_dict(self):
> + return dict(type=self.type, offset=self.offset)
> +
>
> class Qcow2BitmapTable:
>
> @@ -230,6 +255,18 @@ class Qcow2BitmapTable:
> for i, entry in bitmap_table:
> print(f'{i:<14} {entry.type:<15} {entry.offset:<24} {size}')
>
> + def get_fields_dict(self):
> + return dict(entries=self.entries)
> +
> +
> +class Qcow2HeaderExtensionsDoc:
> +
> + def __init__(self, extensions):
> + self.extensions = extensions
> +
> + def get_fields_dict(self):
> + return dict(Header_extensions=self.extensions)
> +
>
> QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
>
> @@ -245,6 +282,9 @@ class QcowHeaderExtension(Qcow2Struct):
> 0x44415441: 'Data file'
> }
>
> + def get_fields_dict(self):
> + return self.mapping.get(self.value, "<unknown>")
> +
> fields = (
> ('u32', Magic, 'magic'),
> ('u32', '{}', 'length')
> @@ -303,6 +343,16 @@ class QcowHeaderExtension(Qcow2Struct):
> else:
> self.obj.dump(dump_json)
>
> + def get_fields_dict(self):
> + ext_name = dict(name=self.Magic(self.magic))
> + he_dict = {**ext_name, **self.fields_dict}
> + if self.obj is not None:
> + he_dict.update(data=self.obj)
> + else:
> + he_dict.update(data_str=self.data_str)
> +
> + return he_dict
> +
> @classmethod
> def create(cls, magic, data):
> return QcowHeaderExtension(magic, len(data), data)
> @@ -401,7 +451,16 @@ class QcowHeader(Qcow2Struct):
> fd.write(buf)
>
> def dump_extensions(self, dump_json=None):
> + if dump_json:
> + ext_doc = Qcow2HeaderExtensionsDoc(self.extensions)
> + print(json.dumps(ext_doc.get_fields_dict(), indent=4,
> + cls=ComplexEncoder))
> + return
> +
> for ex in self.extensions:
> print('Header extension:')
> ex.dump(dump_json)
> print()
> +
> + def get_fields_dict(self):
> + return self.fields_dict
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 28+ messages in thread