* [PATCH v2 00/12] qemu-img info: Show protocol-level information
@ 2022-06-20 16:26 Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 01/12] block: Improve empty format-specific info dump Hanna Reitz
` (12 more replies)
0 siblings, 13 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
Hi,
This series is a v2 to:
https://lists.nongnu.org/archive/html/qemu-block/2022-05/msg00042.html
Like v1, the purpose is to have qemu-img info print the extent-size-hint
for images on filesystems that support it. In contrast to v1, it does
so in a more complicated way.
v1 printed this information by adding protocol-specific information to
ImageInfo, which Kevin disliked and instead proposed to have ImageInfo
point to full ImageInfo objects for the respective node’s children.
I considered two possible solutions:
(A) Ignore the proposal, but not add protocol-specific information to
ImageInfo. Instead, have img_info() itself try to find a clearly
identifiable protocol node and print its driver-specific information
alongside the rest of the information.
I discarded that idea because --output=json is supposed to produce a
QAPI type, and so this additional information wouldn’t be there. It
wouldn’t be great to print more information with --output=human than
with --output=json.
(B) Somehow let qemu-img info print the block graph.
This implements (B). I decided against simply putting recursive
ImageInfo objects into ImageInfo, for three reasons:
1. Extremely personal and unsubstantiated opinion: I don’t like it for
block query functions to return a whole graph. I prefer query
functions to work on single nodes and users to manually walk the
graph if they need information about multiple nodes.
2. ImageInfo already has a link to the backing node’s ImageInfo. It
would be strange to link the backing node’s ImageInfo twice (once in
backing-image, once in the list of all child nodes).
3. query-named-block-nodes returns a list of BlockDeviceInfo objects,
and every such object has an ImageInfo field. I think it would be a
mistake for these ImageInfo fields to be full block subgraphs.
Now, query-named-block-nodes already has a @flat parameter that can
be used to suppress the backing-image information that is in
ImageInfo. Still, it would be a bad idea to surprise users that
don’t set it to true (it defaults to false) by blowing up the data
they get back. We could add another parameter @nested that needs to
be explicitly set to true or users will not get the child
information; but having both @flat and @nested is kind of a bad
interface.
So I decided to split a new structure BlockNodeInfo off of ImageInfo,
where BlockNodeInfo contains everything that ImageInfo did except for
the backing-image link. We can then create another structure
BlockGraphInfo that builds on BlockNodeInfo, and in contrast to
ImageInfo has link to all children instead of just backing-image. We
then let qemu-img info output BlockGraphInfo objects, which works well
because qemu-img info has always ensured the backing-image field would
not be used (so the ImageInfo objects it emitted effectively always were
what are now BlockNodeInfo objects).
(I hope this reorganization isn’t an incompatible change, but I’m not
sure, I have to admit...)
This gets around having to deal with QMP changes relating to ImageInfo
or BlockDeviceInfo (e.g. with query-named-block-nodes), and we don’t
have to worry about the fact that the backing node’s ImageInfo were
duplicated.
There is another potential duplication problem, though: VMDK’s
format-specific info (ImageInfoSpecificVmdk) contains an array of
ImageInfo objects for its extent files. Just like with backing-image,
it seems not great to duplicate these links.
On closer inspection, however, it turns out that these objects aren’t
actually the extent nodes’ ImageInfo data at all. These objects are
filled by the VMDK driver with custom information that actually does not
fit the ImageInfo structure, for example, the @format field is not a
block driver type, but a VMDK format, like "SPARSE" or "FLAT".
Therefore, the child links in the new BlockGraphInfo object actually
would not link to duplicate information.
I decided to make that explicit by changing the extent information type
from ImageInfo to a new VmdkExtentInfo type. I don’t know whether that
is technically an incompatible change, and I don’t know whether it even
matters. We can skip that type change, and this series should still
work, but I feel like it would have been better for these objects to
have had their own type from the start.
So the final state is that despite the QAPI changes, hopefully only the
qemu-img info output changes, and it now prints every image node’s
subgraph. This results in an output like the following (for a qcow2
image):
image: test.qcow2
file format: qcow2
virtual size: 64 MiB (67108864 bytes)
disk size: 196 KiB
cluster_size: 65536
Format specific information:
compat: 1.1
compression type: zlib
lazy refcounts: false
refcount bits: 16
corrupt: false
extended l2: false
Child node '/file':
filename: test.qcow2
protocol type: file
file length: 192 KiB (197120 bytes)
disk size: 196 KiB
Format specific information:
extent size hint: 1048576
For reference, the output from v1 was this (with “extent size†corrected
to “extent size hintâ€):
image: test.qcow2
file format: qcow2
virtual size: 64 MiB (67108864 bytes)
disk size: 196 KiB
cluster_size: 65536
Format specific information:
compat: 1.1
compression type: zlib
lazy refcounts: false
refcount bits: 16
corrupt: false
extended l2: false
Protocol specific information:
extent size hint: 1048576
I think I still slightly prefer the output from v1, because the
additional information is mostly just duplicated information (and thus
clutters the output), but I can see that hard-adding protocol-specific
information to ImageInfo wasn’t a good way to go about it (and I can’t
find any better reasonable way to only print that driver-specific
information (and nothing else) from the protocol node).
For completeness’s sake, git-backport-diff to v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/12:[----] [--] 'block: Improve empty format-specific info dump'
002/12:[0008] [FC] 'block/file: Add file-specific image info'
003/12:[down] 'block/vmdk: Change extent info type'
004/12:[down] 'block: Split BlockNodeInfo off of ImageInfo'
005/12:[down] 'qemu-img: Use BlockNodeInfo'
006/12:[down] 'block/qapi: Let bdrv_query_image_info() recurse'
007/12:[down] 'block/qapi: Introduce BlockGraphInfo'
008/12:[down] 'block/qapi: Add indentation to bdrv_node_info_dump()'
009/12:[down] 'iotests: Filter child node information'
010/12:[down] 'iotests/106, 214, 308: Read only one size line'
011/12:[down] 'qemu-img: Let info print block graph'
012/12:[down] 'qemu-img: Change info key names for protocol nodes'
Hanna Reitz (12):
block: Improve empty format-specific info dump
block/file: Add file-specific image info
block/vmdk: Change extent info type
block: Split BlockNodeInfo off of ImageInfo
qemu-img: Use BlockNodeInfo
block/qapi: Let bdrv_query_image_info() recurse
block/qapi: Introduce BlockGraphInfo
block/qapi: Add indentation to bdrv_node_info_dump()
iotests: Filter child node information
iotests/106, 214, 308: Read only one size line
qemu-img: Let info print block graph
qemu-img: Change info key names for protocol nodes
qapi/block-core.json | 121 +++++++++++-
include/block/qapi.h | 14 +-
block/file-posix.c | 30 +++
block/monitor/block-hmp-cmds.c | 2 +-
block/qapi.c | 319 ++++++++++++++++++++++++-------
block/vmdk.c | 8 +-
qemu-img.c | 76 +++++---
qemu-io-cmds.c | 5 +-
tests/qemu-iotests/065 | 2 +-
tests/qemu-iotests/106 | 4 +-
tests/qemu-iotests/214 | 6 +-
tests/qemu-iotests/302.out | 5 +
tests/qemu-iotests/308 | 4 +-
tests/qemu-iotests/common.filter | 22 ++-
tests/qemu-iotests/common.rc | 22 ++-
tests/qemu-iotests/iotests.py | 18 +-
16 files changed, 519 insertions(+), 139 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 01/12] block: Improve empty format-specific info dump
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2023-01-19 14:00 ` Kevin Wolf
2022-06-20 16:26 ` [PATCH v2 02/12] block/file: Add file-specific image info Hanna Reitz
` (11 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
When a block driver supports obtaining format-specific information, but
that object only contains optional fields, it is possible that none of
them are present, so that dump_qobject() (called by
bdrv_image_info_specific_dump()) will not print anything.
The callers of bdrv_image_info_specific_dump() put a header above this
information ("Format specific information:\n"), which will look strange
when there is nothing below. Modify bdrv_image_info_specific_dump() to
print this header instead of its callers, and only if there is indeed
something to be printed.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
include/block/qapi.h | 3 ++-
block/qapi.c | 41 +++++++++++++++++++++++++++++++++++++----
qemu-io-cmds.c | 4 ++--
3 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 22c7807c89..c09859ea78 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -40,6 +40,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
Error **errp);
void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
-void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec);
+void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
+ const char *prefix);
void bdrv_image_info_dump(ImageInfo *info);
#endif
diff --git a/block/qapi.c b/block/qapi.c
index cf557e3aea..51202b470a 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -777,7 +777,35 @@ static void dump_qdict(int indentation, QDict *dict)
}
}
-void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec)
+/*
+ * Return whether dumping the given QObject with dump_qobject() would
+ * yield an empty dump, i.e. not print anything.
+ */
+static bool qobject_is_empty_dump(const QObject *obj)
+{
+ switch (qobject_type(obj)) {
+ case QTYPE_QNUM:
+ case QTYPE_QSTRING:
+ case QTYPE_QBOOL:
+ return false;
+
+ case QTYPE_QDICT:
+ return qdict_size(qobject_to(QDict, obj)) == 0;
+
+ case QTYPE_QLIST:
+ return qlist_empty(qobject_to(QList, obj));
+
+ default:
+ abort();
+ }
+}
+
+/**
+ * Dumps the given ImageInfoSpecific object in a human-readable form,
+ * prepending an optional prefix if the dump is not empty.
+ */
+void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
+ const char *prefix)
{
QObject *obj, *data;
Visitor *v = qobject_output_visitor_new(&obj);
@@ -785,7 +813,12 @@ void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec)
visit_type_ImageInfoSpecific(v, NULL, &info_spec, &error_abort);
visit_complete(v, &obj);
data = qdict_get(qobject_to(QDict, obj), "data");
- dump_qobject(1, data);
+ if (!qobject_is_empty_dump(data)) {
+ if (prefix) {
+ qemu_printf("%s", prefix);
+ }
+ dump_qobject(1, data);
+ }
qobject_unref(obj);
visit_free(v);
}
@@ -866,7 +899,7 @@ void bdrv_image_info_dump(ImageInfo *info)
}
if (info->has_format_specific) {
- qemu_printf("Format specific information:\n");
- bdrv_image_info_specific_dump(info->format_specific);
+ bdrv_image_info_specific_dump(info->format_specific,
+ "Format specific information:\n");
}
}
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2f0d8ac25a..084ec44d3b 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1819,8 +1819,8 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
return -EIO;
}
if (spec_info) {
- printf("Format specific information:\n");
- bdrv_image_info_specific_dump(spec_info);
+ bdrv_image_info_specific_dump(spec_info,
+ "Format specific information:\n");
qapi_free_ImageInfoSpecific(spec_info);
}
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 02/12] block/file: Add file-specific image info
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 01/12] block: Improve empty format-specific info dump Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2023-01-27 23:00 ` Eric Blake
2022-06-20 16:26 ` [PATCH v2 03/12] block/vmdk: Change extent info type Hanna Reitz
` (10 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
Add some (optional) information that the file driver can provide for
image files, namely the extent size hint.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
qapi/block-core.json | 26 ++++++++++++++++++++++++--
block/file-posix.c | 30 ++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 457df16638..40fb307e2d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -139,16 +139,29 @@
'*encryption-format': 'RbdImageEncryptionFormat'
} }
+##
+# @ImageInfoSpecificFile:
+#
+# @extent-size-hint: Extent size hint (if available)
+#
+# Since: 7.1
+##
+{ 'struct': 'ImageInfoSpecificFile',
+ 'data': {
+ '*extent-size-hint': 'size'
+ } }
+
##
# @ImageInfoSpecificKind:
#
# @luks: Since 2.7
# @rbd: Since 6.1
+# @file: Since 7.1
#
# Since: 1.7
##
{ 'enum': 'ImageInfoSpecificKind',
- 'data': [ 'qcow2', 'vmdk', 'luks', 'rbd' ] }
+ 'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file' ] }
##
# @ImageInfoSpecificQCow2Wrapper:
@@ -185,6 +198,14 @@
{ 'struct': 'ImageInfoSpecificRbdWrapper',
'data': { 'data': 'ImageInfoSpecificRbd' } }
+##
+# @ImageInfoSpecificFileWrapper:
+#
+# Since: 7.1
+##
+{ 'struct': 'ImageInfoSpecificFileWrapper',
+ 'data': { 'data': 'ImageInfoSpecificFile' } }
+
##
# @ImageInfoSpecific:
#
@@ -199,7 +220,8 @@
'qcow2': 'ImageInfoSpecificQCow2Wrapper',
'vmdk': 'ImageInfoSpecificVmdkWrapper',
'luks': 'ImageInfoSpecificLUKSWrapper',
- 'rbd': 'ImageInfoSpecificRbdWrapper'
+ 'rbd': 'ImageInfoSpecificRbdWrapper',
+ 'file': 'ImageInfoSpecificFileWrapper'
} }
##
diff --git a/block/file-posix.c b/block/file-posix.c
index 48cd096624..74a649b409 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -3083,6 +3083,34 @@ static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
+static ImageInfoSpecific *raw_get_specific_info(BlockDriverState *bs,
+ Error **errp)
+{
+ BDRVRawState *s = bs->opaque;
+ ImageInfoSpecificFile *file_info = g_new0(ImageInfoSpecificFile, 1);
+ ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
+
+ *spec_info = (ImageInfoSpecific){
+ .type = IMAGE_INFO_SPECIFIC_KIND_FILE,
+ .u.file.data = file_info,
+ };
+
+#ifdef FS_IOC_FSGETXATTR
+ {
+ struct fsxattr attr;
+ int ret;
+
+ ret = ioctl(s->fd, FS_IOC_FSGETXATTR, &attr);
+ if (!ret && attr.fsx_extsize != 0) {
+ file_info->has_extent_size_hint = true;
+ file_info->extent_size_hint = attr.fsx_extsize;
+ }
+ }
+#endif
+
+ return spec_info;
+}
+
static BlockStatsSpecificFile get_blockstats_specific_file(BlockDriverState *bs)
{
BDRVRawState *s = bs->opaque;
@@ -3316,6 +3344,7 @@ BlockDriver bdrv_file = {
.bdrv_co_truncate = raw_co_truncate,
.bdrv_getlength = raw_getlength,
.bdrv_get_info = raw_get_info,
+ .bdrv_get_specific_info = raw_get_specific_info,
.bdrv_get_allocated_file_size
= raw_get_allocated_file_size,
.bdrv_get_specific_stats = raw_get_specific_stats,
@@ -3688,6 +3717,7 @@ static BlockDriver bdrv_host_device = {
.bdrv_co_truncate = raw_co_truncate,
.bdrv_getlength = raw_getlength,
.bdrv_get_info = raw_get_info,
+ .bdrv_get_specific_info = raw_get_specific_info,
.bdrv_get_allocated_file_size
= raw_get_allocated_file_size,
.bdrv_get_specific_stats = hdev_get_specific_stats,
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 03/12] block/vmdk: Change extent info type
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 01/12] block: Improve empty format-specific info dump Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 02/12] block/file: Add file-specific image info Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2023-01-19 15:20 ` Kevin Wolf
2022-06-20 16:26 ` [PATCH v2 04/12] block: Split BlockNodeInfo off of ImageInfo Hanna Reitz
` (9 subsequent siblings)
12 siblings, 1 reply; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
VMDK's implementation of .bdrv_get_specific_info() returns information
about its extent files, ostensibly in the form of ImageInfo objects.
However, it does not get this information through
bdrv_query_image_info(), but fills only a select few fields with custom
information that does not always match the fields' purposes.
For example, @format, which is supposed to be a block driver name, is
filled with the extent type, e.g. SPARSE or FLAT.
In ImageInfo, @compressed shows whether the data that can be seen in the
image is stored in compressed form or not. For example, a compressed
qcow2 image will store compressed data in its data file, but when
accessing the qcow2 node, you will see normal data. This is not how
VMDK uses the @compressed field for its extent files: Instead, it
signifies whether accessing the extent file will yield compressed data
(which the VMDK driver then (de-)compresses).
Create a new structure to represent the extent information. This allows
us to clarify the fields' meanings, and it clearly shows that these are
not complete ImageInfo objects. (That is, if a user wants an extent
file's ImageInfo object, they will need to query it separately, and will
not get it from ImageInfoSpecificVmdk.extents.)
Note that this removes the last use of ['ImageInfo'] (i.e. an array of
ImageInfo objects), so the QAPI generator will no longer generate
ImageInfoList by default. However, we use it in qemu-img.c, so we need
to create a dummy object to force the generate to create that type,
similarly to DummyForceArrays in machine.json (introduced in commit
9f08c8ec73878122ad4b061ed334f0437afaaa32 ("qapi: Lazy creation of array
types")).
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
I'm not sure whether this is an incompatible change. I'm also not sure
if it even matters whether it's an incompatible change (i.e. whether
anyone cares).
I believe we can get away without this change. I find it useful to make
it clear that (A) this extent information is not what you would find in
other ImageInfo objects (i.e., I consider this a fix for
ImageInfoSpecificVmdk's @extents field), and (B) that the upcoming
BlockGraphInfo type will not duplicate the extent nodes' ImageInfo
information, because those are actual ImageInfo objects.
We can probably replace this patch by clarifying all of this in
documentation, but if possible I would prefer a syntactic clarification
(as done here).
---
qapi/block-core.json | 38 +++++++++++++++++++++++++++++++++++++-
block/vmdk.c | 8 ++++----
2 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 40fb307e2d..e0c8f07932 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -124,7 +124,33 @@
'create-type': 'str',
'cid': 'int',
'parent-cid': 'int',
- 'extents': ['ImageInfo']
+ 'extents': ['VmdkExtentInfo']
+ } }
+
+##
+# @VmdkExtentInfo:
+#
+# Information about a VMDK extent file
+#
+# @filename: Name of the extent file
+#
+# @format: Extent type (e.g. FLAT or SPARSE)
+#
+# @virtual-size: Number of bytes covered by this extent
+#
+# @cluster-size: Cluster size in bytes (for non-flat extents)
+#
+# @compressed: Whether this extent contains compressed data
+#
+# Since: 7.1
+##
+{ 'struct': 'VmdkExtentInfo',
+ 'data': {
+ 'filename': 'str',
+ 'format': 'str',
+ 'virtual-size': 'int',
+ '*cluster-size': 'int',
+ '*compressed': 'bool'
} }
##
@@ -5638,3 +5664,13 @@
'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
'returns': 'SnapshotInfo',
'allow-preconfig': true }
+
+##
+# @DummyBlockCoreForceArrays:
+#
+# Not used by QMP; hack to let us use ImageInfoList internally
+#
+# Since: 7.1
+##
+{ 'struct': 'DummyBlockCoreForceArrays',
+ 'data': { 'unused-image-info': ['ImageInfo'] } }
diff --git a/block/vmdk.c b/block/vmdk.c
index 38e5ab3806..35131a916e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2908,12 +2908,12 @@ static int vmdk_has_zero_init(BlockDriverState *bs)
return 1;
}
-static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
+static VmdkExtentInfo *vmdk_get_extent_info(VmdkExtent *extent)
{
- ImageInfo *info = g_new0(ImageInfo, 1);
+ VmdkExtentInfo *info = g_new0(VmdkExtentInfo, 1);
bdrv_refresh_filename(extent->file->bs);
- *info = (ImageInfo){
+ *info = (VmdkExtentInfo){
.filename = g_strdup(extent->file->bs->filename),
.format = g_strdup(extent->type),
.virtual_size = extent->sectors * BDRV_SECTOR_SIZE,
@@ -2992,7 +2992,7 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs,
int i;
BDRVVmdkState *s = bs->opaque;
ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific, 1);
- ImageInfoList **tail;
+ VmdkExtentInfoList **tail;
*spec_info = (ImageInfoSpecific){
.type = IMAGE_INFO_SPECIFIC_KIND_VMDK,
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 04/12] block: Split BlockNodeInfo off of ImageInfo
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (2 preceding siblings ...)
2022-06-20 16:26 ` [PATCH v2 03/12] block/vmdk: Change extent info type Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 05/12] qemu-img: Use BlockNodeInfo Hanna Reitz
` (8 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
ImageInfo sometimes contains flat information, and sometimes it does
not. Split off a BlockNodeInfo struct, which only contains information
about a single node and has no link to the backing image.
We do this so we can extend BlockNodeInfo to a BlockGraphInfo struct,
which has links to all child nodes, not just the backing node. It would
be strange to base BlockGraphInfo on ImageInfo, because then this
extended struct would have two links to the backing node (one in
BlockGraphInfo as one of all the child links, and one in ImageInfo).
Furthermore, it is quite common to ignore the backing-image field
altogether: bdrv_query_image_info() does not set it, and
bdrv_image_info_dump() does not evaluate it. That signals that we
should have different structs for describing a single node and one that
has a link to the backing image.
Still, bdrv_query_image_info() and bdrv_image_info_dump() are not
changed too much in this patch. Follow-up patches will handle them.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
I hope this is not an incompatible change, but I'm not sure, I have to
admit.
---
qapi/block-core.json | 22 +++++++++---
include/block/qapi.h | 3 ++
block/qapi.c | 86 ++++++++++++++++++++++++++++++++------------
3 files changed, 84 insertions(+), 27 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e0c8f07932..8c06deacdf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -251,7 +251,7 @@
} }
##
-# @ImageInfo:
+# @BlockNodeInfo:
#
# Information about a QEMU image file
#
@@ -279,22 +279,34 @@
#
# @snapshots: list of VM snapshots
#
-# @backing-image: info of the backing image (since 1.6)
-#
# @format-specific: structure supplying additional format-specific
# information (since 1.7)
#
# Since: 1.3
##
-{ 'struct': 'ImageInfo',
+{ 'struct': 'BlockNodeInfo',
'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
'*actual-size': 'int', 'virtual-size': 'int',
'*cluster-size': 'int', '*encrypted': 'bool', '*compressed': 'bool',
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
- '*backing-image': 'ImageInfo',
'*format-specific': 'ImageInfoSpecific' } }
+##
+# @ImageInfo:
+#
+# Information about a QEMU image file, and potentially its backing image
+#
+# @backing-image: info of the backing image
+#
+# Since: 7.1
+##
+{ 'struct': 'ImageInfo',
+ 'base': 'BlockNodeInfo',
+ 'data': {
+ '*backing-image': 'ImageInfo'
+ } }
+
##
# @ImageCheck:
#
diff --git a/include/block/qapi.h b/include/block/qapi.h
index c09859ea78..c7de4e3fa9 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -35,6 +35,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
int bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
Error **errp);
+void bdrv_query_block_node_info(BlockDriverState *bs,
+ BlockNodeInfo **p_info,
+ Error **errp);
void bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
Error **errp);
diff --git a/block/qapi.c b/block/qapi.c
index 51202b470a..e5022b4481 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -241,30 +241,18 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
}
/**
- * bdrv_query_image_info:
- * @bs: block device to examine
- * @p_info: location to store image information
- * @errp: location to store error information
- *
- * Store "flat" image information in @p_info.
- *
- * "Flat" means it does *not* query backing image information,
- * i.e. (*pinfo)->has_backing_image will be set to false and
- * (*pinfo)->backing_image to NULL even when the image does in fact have
- * a backing image.
- *
- * @p_info will be set only on success. On error, store error in @errp.
+ * Helper function for other query info functions. Store information about @bs
+ * in @info, setting @errp on error.
*/
-void bdrv_query_image_info(BlockDriverState *bs,
- ImageInfo **p_info,
- Error **errp)
+static void bdrv_do_query_node_info(BlockDriverState *bs,
+ BlockNodeInfo *info,
+ Error **errp)
{
int64_t size;
const char *backing_filename;
BlockDriverInfo bdi;
int ret;
Error *err = NULL;
- ImageInfo *info;
aio_context_acquire(bdrv_get_aio_context(bs));
@@ -277,7 +265,6 @@ void bdrv_query_image_info(BlockDriverState *bs,
bdrv_refresh_filename(bs);
- info = g_new0(ImageInfo, 1);
info->filename = g_strdup(bs->filename);
info->format = g_strdup(bdrv_get_format_name(bs));
info->virtual_size = size;
@@ -298,7 +285,6 @@ void bdrv_query_image_info(BlockDriverState *bs,
info->format_specific = bdrv_get_specific_info(bs, &err);
if (err) {
error_propagate(errp, err);
- qapi_free_ImageInfo(info);
goto out;
}
info->has_format_specific = info->format_specific != NULL;
@@ -339,16 +325,72 @@ void bdrv_query_image_info(BlockDriverState *bs,
break;
default:
error_propagate(errp, err);
- qapi_free_ImageInfo(info);
goto out;
}
- *p_info = info;
-
out:
aio_context_release(bdrv_get_aio_context(bs));
}
+/**
+ * bdrv_query_block_node_info:
+ * @bs: block node to examine
+ * @p_info: location to store node information
+ * @errp: location to store error information
+ *
+ * Store image information about @bs in @p_info.
+ *
+ * @p_info will be set only on success. On error, store error in @errp.
+ */
+void bdrv_query_block_node_info(BlockDriverState *bs,
+ BlockNodeInfo **p_info,
+ Error **errp)
+{
+ BlockNodeInfo *info;
+ ERRP_GUARD();
+
+ info = g_new0(BlockNodeInfo, 1);
+ bdrv_do_query_node_info(bs, info, errp);
+ if (*errp) {
+ qapi_free_BlockNodeInfo(info);
+ return;
+ }
+
+ *p_info = info;
+}
+
+/**
+ * bdrv_query_image_info:
+ * @bs: block node to examine
+ * @p_info: location to store image information
+ * @errp: location to store error information
+ *
+ * Store "flat" image information in @p_info.
+ *
+ * "Flat" means it does *not* query backing image information,
+ * i.e. (*pinfo)->has_backing_image will be set to false and
+ * (*pinfo)->backing_image to NULL even when the image does in fact have
+ * a backing image.
+ *
+ * @p_info will be set only on success. On error, store error in @errp.
+ */
+void bdrv_query_image_info(BlockDriverState *bs,
+ ImageInfo **p_info,
+ Error **errp)
+{
+ ImageInfo *info;
+ ERRP_GUARD();
+
+ info = g_new0(ImageInfo, 1);
+ bdrv_do_query_node_info(bs, qapi_ImageInfo_base(info), errp);
+ if (*errp) {
+ qapi_free_ImageInfo(info);
+ return;
+ }
+
+ *p_info = info;
+}
+
/* @p_info will be set only on success. */
static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
Error **errp)
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 05/12] qemu-img: Use BlockNodeInfo
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (3 preceding siblings ...)
2022-06-20 16:26 ` [PATCH v2 04/12] block: Split BlockNodeInfo off of ImageInfo Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 06/12] block/qapi: Let bdrv_query_image_info() recurse Hanna Reitz
` (7 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
qemu-img info never uses ImageInfo's backing-image field, because it
opens the backing chain one by one with BDRV_O_NO_BACKING, and prints
all backing chain nodes' information consecutively. Use BlockNodeInfo
to make it clear that we only print information about a single node, and
that we are not using the backing-image field.
Notably, bdrv_image_info_dump() does not evaluate the backing-image
field, so we can easily make it take a BlockNodeInfo pointer (and
consequentially rename it to bdrv_node_info_dump()). It makes more
sense this way, because again, the interface now makes it syntactically
clear that backing-image is ignored by this function.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
qapi/block-core.json | 4 +--
include/block/qapi.h | 2 +-
block/monitor/block-hmp-cmds.c | 2 +-
block/qapi.c | 2 +-
qemu-img.c | 48 +++++++++++++++++-----------------
5 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8c06deacdf..d5d1c8ff15 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5680,9 +5680,9 @@
##
# @DummyBlockCoreForceArrays:
#
-# Not used by QMP; hack to let us use ImageInfoList internally
+# Not used by QMP; hack to let us use BlockNodeInfoList internally
#
# Since: 7.1
##
{ 'struct': 'DummyBlockCoreForceArrays',
- 'data': { 'unused-image-info': ['ImageInfo'] } }
+ 'data': { 'unused-block-node-info': ['BlockNodeInfo'] } }
diff --git a/include/block/qapi.h b/include/block/qapi.h
index c7de4e3fa9..22198dcd0c 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -45,5 +45,5 @@ void bdrv_query_image_info(BlockDriverState *bs,
void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
const char *prefix);
-void bdrv_image_info_dump(ImageInfo *info);
+void bdrv_node_info_dump(BlockNodeInfo *info);
#endif
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bfb3c043a0..d286f2d1c5 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -734,7 +734,7 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
monitor_printf(mon, "\nImages:\n");
image_info = inserted->image;
while (1) {
- bdrv_image_info_dump(image_info);
+ bdrv_node_info_dump(qapi_ImageInfo_base(image_info));
if (image_info->has_backing_image) {
image_info = image_info->backing_image;
} else {
diff --git a/block/qapi.c b/block/qapi.c
index e5022b4481..ad88bf9b38 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -865,7 +865,7 @@ void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
visit_free(v);
}
-void bdrv_image_info_dump(ImageInfo *info)
+void bdrv_node_info_dump(BlockNodeInfo *info)
{
char *size_buf, *dsize_buf;
if (!info->has_actual_size) {
diff --git a/qemu-img.c b/qemu-img.c
index 4cf4d2423d..3b1f950c85 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2800,13 +2800,13 @@ static void dump_snapshots(BlockDriverState *bs)
g_free(sn_tab);
}
-static void dump_json_image_info_list(ImageInfoList *list)
+static void dump_json_block_node_info_list(BlockNodeInfoList *list)
{
GString *str;
QObject *obj;
Visitor *v = qobject_output_visitor_new(&obj);
- visit_type_ImageInfoList(v, NULL, &list, &error_abort);
+ visit_type_BlockNodeInfoList(v, NULL, &list, &error_abort);
visit_complete(v, &obj);
str = qobject_to_json_pretty(obj, true);
assert(str != NULL);
@@ -2816,13 +2816,13 @@ static void dump_json_image_info_list(ImageInfoList *list)
g_string_free(str, true);
}
-static void dump_json_image_info(ImageInfo *info)
+static void dump_json_block_node_info(BlockNodeInfo *info)
{
GString *str;
QObject *obj;
Visitor *v = qobject_output_visitor_new(&obj);
- visit_type_ImageInfo(v, NULL, &info, &error_abort);
+ visit_type_BlockNodeInfo(v, NULL, &info, &error_abort);
visit_complete(v, &obj);
str = qobject_to_json_pretty(obj, true);
assert(str != NULL);
@@ -2832,9 +2832,9 @@ static void dump_json_image_info(ImageInfo *info)
g_string_free(str, true);
}
-static void dump_human_image_info_list(ImageInfoList *list)
+static void dump_human_image_info_list(BlockNodeInfoList *list)
{
- ImageInfoList *elem;
+ BlockNodeInfoList *elem;
bool delim = false;
for (elem = list; elem; elem = elem->next) {
@@ -2843,7 +2843,7 @@ static void dump_human_image_info_list(ImageInfoList *list)
}
delim = true;
- bdrv_image_info_dump(elem->value);
+ bdrv_node_info_dump(elem->value);
}
}
@@ -2853,24 +2853,24 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
}
/**
- * Open an image file chain and return an ImageInfoList
+ * Open an image file chain and return an BlockNodeInfoList
*
* @filename: topmost image filename
* @fmt: topmost image format (may be NULL to autodetect)
* @chain: true - enumerate entire backing file chain
* false - only topmost image file
*
- * Returns a list of ImageInfo objects or NULL if there was an error opening an
- * image file. If there was an error a message will have been printed to
- * stderr.
+ * Returns a list of BlockNodeInfo objects or NULL if there was an error
+ * opening an image file. If there was an error a message will have been
+ * printed to stderr.
*/
-static ImageInfoList *collect_image_info_list(bool image_opts,
- const char *filename,
- const char *fmt,
- bool chain, bool force_share)
+static BlockNodeInfoList *collect_image_info_list(bool image_opts,
+ const char *filename,
+ const char *fmt,
+ bool chain, bool force_share)
{
- ImageInfoList *head = NULL;
- ImageInfoList **tail = &head;
+ BlockNodeInfoList *head = NULL;
+ BlockNodeInfoList **tail = &head;
GHashTable *filenames;
Error *err = NULL;
@@ -2879,7 +2879,7 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
while (filename) {
BlockBackend *blk;
BlockDriverState *bs;
- ImageInfo *info;
+ BlockNodeInfo *info;
if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
error_report("Backing file '%s' creates an infinite loop.",
@@ -2896,7 +2896,7 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
}
bs = blk_bs(blk);
- bdrv_query_image_info(bs, &info, &err);
+ bdrv_query_block_node_info(bs, &info, &err);
if (err) {
error_report_err(err);
blk_unref(blk);
@@ -2929,7 +2929,7 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
return head;
err:
- qapi_free_ImageInfoList(head);
+ qapi_free_BlockNodeInfoList(head);
g_hash_table_destroy(filenames);
return NULL;
}
@@ -2940,7 +2940,7 @@ static int img_info(int argc, char **argv)
OutputFormat output_format = OFORMAT_HUMAN;
bool chain = false;
const char *filename, *fmt, *output;
- ImageInfoList *list;
+ BlockNodeInfoList *list;
bool image_opts = false;
bool force_share = false;
@@ -3019,14 +3019,14 @@ static int img_info(int argc, char **argv)
break;
case OFORMAT_JSON:
if (chain) {
- dump_json_image_info_list(list);
+ dump_json_block_node_info_list(list);
} else {
- dump_json_image_info(list->value);
+ dump_json_block_node_info(list->value);
}
break;
}
- qapi_free_ImageInfoList(list);
+ qapi_free_BlockNodeInfoList(list);
return 0;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 06/12] block/qapi: Let bdrv_query_image_info() recurse
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (4 preceding siblings ...)
2022-06-20 16:26 ` [PATCH v2 05/12] qemu-img: Use BlockNodeInfo Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 07/12] block/qapi: Introduce BlockGraphInfo Hanna Reitz
` (6 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
There is no real reason why bdrv_query_image_info() should generally not
recurse. The ImageInfo struct has a pointer to the backing image, so it
should generally be filled, unless the caller explicitly opts out.
This moves the recursing code from bdrv_block_device_info() into
bdrv_query_image_info().
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
include/block/qapi.h | 2 +
block/qapi.c | 94 +++++++++++++++++++++++++++-----------------
2 files changed, 59 insertions(+), 37 deletions(-)
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 22198dcd0c..2174bf8fa2 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -40,6 +40,8 @@ void bdrv_query_block_node_info(BlockDriverState *bs,
Error **errp);
void bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
+ bool flat,
+ bool skip_implicit_filters,
Error **errp);
void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
diff --git a/block/qapi.c b/block/qapi.c
index ad88bf9b38..5d0a8d2ce3 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -47,8 +47,10 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
Error **errp)
{
ImageInfo **p_image_info;
+ ImageInfo *backing_info;
BlockDriverState *bs0, *backing;
BlockDeviceInfo *info;
+ ERRP_GUARD();
if (!bs->drv) {
error_setg(errp, "Block device %s is ejected", bs->node_name);
@@ -149,38 +151,21 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
bs0 = bs;
p_image_info = &info->image;
info->backing_file_depth = 0;
- while (1) {
- Error *local_err = NULL;
- bdrv_query_image_info(bs0, p_image_info, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- qapi_free_BlockDeviceInfo(info);
- return NULL;
- }
-
- /* stop gathering data for flat output */
- if (flat) {
- break;
- }
- if (bs0->drv && bdrv_filter_or_cow_child(bs0)) {
- /*
- * Put any filtered child here (for backwards compatibility to when
- * we put bs0->backing here, which might be any filtered child).
- */
- info->backing_file_depth++;
- bs0 = bdrv_filter_or_cow_bs(bs0);
- (*p_image_info)->has_backing_image = true;
- p_image_info = &((*p_image_info)->backing_image);
- } else {
- break;
- }
+ /*
+ * Skip automatically inserted nodes that the user isn't aware of for
+ * query-block (blk != NULL), but not for query-named-block-nodes
+ */
+ bdrv_query_image_info(bs0, p_image_info, flat, blk != NULL, errp);
+ if (*errp) {
+ qapi_free_BlockDeviceInfo(info);
+ return NULL;
+ }
- /* Skip automatically inserted nodes that the user isn't aware of for
- * query-block (blk != NULL), but not for query-named-block-nodes */
- if (blk) {
- bs0 = bdrv_skip_implicit_filters(bs0);
- }
+ backing_info = info->image->backing_image;
+ while (backing_info) {
+ info->backing_file_depth++;
+ backing_info = backing_info->backing_image;
}
return info;
@@ -363,19 +348,28 @@ void bdrv_query_block_node_info(BlockDriverState *bs,
* bdrv_query_image_info:
* @bs: block node to examine
* @p_info: location to store image information
+ * @flat: skip backing node information
+ * @skip_implicit_filters: skip implicit filters in the backing chain
* @errp: location to store error information
*
- * Store "flat" image information in @p_info.
+ * Store image information in @p_info, potentially recursively covering the
+ * backing chain.
*
- * "Flat" means it does *not* query backing image information,
- * i.e. (*pinfo)->has_backing_image will be set to false and
- * (*pinfo)->backing_image to NULL even when the image does in fact have
- * a backing image.
+ * If @flat is true, do not query backing image information, i.e.
+ * (*p_info)->has_backing_image will be set to false and
+ * (*p_info)->backing_image to NULL even when the image does in fact have a
+ * backing image.
+ *
+ * If @skip_implicit_filters is true, implicit filter nodes in the backing chain
+ * will be skipped when querying backing image information.
+ * (@skip_implicit_filters is ignored when @flat is true.)
*
* @p_info will be set only on success. On error, store error in @errp.
*/
void bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
+ bool flat,
+ bool skip_implicit_filters,
Error **errp)
{
ImageInfo *info;
@@ -384,11 +378,37 @@ void bdrv_query_image_info(BlockDriverState *bs,
info = g_new0(ImageInfo, 1);
bdrv_do_query_node_info(bs, qapi_ImageInfo_base(info), errp);
if (*errp) {
- qapi_free_ImageInfo(info);
- return;
+ goto fail;
+ }
+
+ if (!flat) {
+ BlockDriverState *backing;
+
+ /*
+ * Use any filtered child here (for backwards compatibility to when
+ * we always took bs->backing, which might be any filtered child).
+ */
+ backing = bdrv_filter_or_cow_bs(bs);
+ if (skip_implicit_filters) {
+ backing = bdrv_skip_implicit_filters(backing);
+ }
+
+ if (backing) {
+ bdrv_query_image_info(backing, &info->backing_image, false,
+ skip_implicit_filters, errp);
+ if (*errp) {
+ goto fail;
+ }
+ info->has_backing_image = true;
+ }
}
*p_info = info;
+ return;
+
+fail:
+ assert(*errp);
+ qapi_free_ImageInfo(info);
}
/* @p_info will be set only on success. */
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 07/12] block/qapi: Introduce BlockGraphInfo
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (5 preceding siblings ...)
2022-06-20 16:26 ` [PATCH v2 06/12] block/qapi: Let bdrv_query_image_info() recurse Hanna Reitz
@ 2022-06-20 16:26 ` Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 08/12] block/qapi: Add indentation to bdrv_node_info_dump() Hanna Reitz
` (5 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:26 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
Introduce a new QAPI type BlockGraphInfo and an associated
bdrv_query_block_graph_info() function that recursively gathers
BlockNodeInfo objects through a block graph.
A follow-up patch is going to make "qemu-img info" use this to print
information about all nodes that are (usually implicitly) opened for a
given image file.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
qapi/block-core.json | 35 ++++++++++++++++++++++++++++++++
include/block/qapi.h | 3 +++
block/qapi.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 86 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d5d1c8ff15..b7e5708487 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -307,6 +307,41 @@
'*backing-image': 'ImageInfo'
} }
+##
+# @BlockChildInfo:
+#
+# Information about all nodes in the block graph starting at some node,
+# annotated with information about that node in relation to its parent.
+#
+# @name: Child name of the root node in the BlockGraphInfo struct, in its role
+# as the child of some undescribed parent node
+#
+# @info: Block graph information starting at this node
+#
+# Since: 7.1
+##
+{ 'struct': 'BlockChildInfo',
+ 'data': {
+ 'name': 'str',
+ 'info': 'BlockGraphInfo'
+ } }
+
+##
+# @BlockGraphInfo:
+#
+# Information about all nodes in a block (sub)graph in the form of BlockNodeInfo
+# data.
+# The base BlockNodeInfo struct contains the information for the (sub)graph's
+# root node.
+#
+# @children: Array of links to this node's child nodes' information
+#
+# Since: 7.1
+##
+{ 'struct': 'BlockGraphInfo',
+ 'base': 'BlockNodeInfo',
+ 'data': { 'children': ['BlockChildInfo'] } }
+
##
# @ImageCheck:
#
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 2174bf8fa2..196436020e 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -43,6 +43,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
bool flat,
bool skip_implicit_filters,
Error **errp);
+void bdrv_query_block_graph_info(BlockDriverState *bs,
+ BlockGraphInfo **p_info,
+ Error **errp);
void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
diff --git a/block/qapi.c b/block/qapi.c
index 5d0a8d2ce3..f208c21ccf 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -411,6 +411,54 @@ fail:
qapi_free_ImageInfo(info);
}
+/**
+ * bdrv_query_block_graph_info:
+ * @bs: root node to start from
+ * @p_info: location to store image information
+ * @errp: location to store error information
+ *
+ * Store image information about the graph starting from @bs in @p_info.
+ *
+ * @p_info will be set only on success. On error, store error in @errp.
+ */
+void bdrv_query_block_graph_info(BlockDriverState *bs,
+ BlockGraphInfo **p_info,
+ Error **errp)
+{
+ BlockGraphInfo *info;
+ BlockChildInfoList **children_list_tail;
+ BdrvChild *c;
+ ERRP_GUARD();
+
+ info = g_new0(BlockGraphInfo, 1);
+ bdrv_do_query_node_info(bs, qapi_BlockGraphInfo_base(info), errp);
+ if (*errp) {
+ goto fail;
+ }
+
+ children_list_tail = &info->children;
+
+ QLIST_FOREACH(c, &bs->children, next) {
+ BlockChildInfo *c_info;
+
+ c_info = g_new0(BlockChildInfo, 1);
+ QAPI_LIST_APPEND(children_list_tail, c_info);
+
+ c_info->name = g_strdup(c->name);
+ bdrv_query_block_graph_info(c->bs, &c_info->info, errp);
+ if (*errp) {
+ goto fail;
+ }
+ }
+
+ *p_info = info;
+ return;
+
+fail:
+ assert(*errp != NULL);
+ qapi_free_BlockGraphInfo(info);
+}
+
/* @p_info will be set only on success. */
static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
Error **errp)
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 08/12] block/qapi: Add indentation to bdrv_node_info_dump()
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (6 preceding siblings ...)
2022-06-20 16:26 ` [PATCH v2 07/12] block/qapi: Introduce BlockGraphInfo Hanna Reitz
@ 2022-06-20 16:27 ` Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 09/12] iotests: Filter child node information Hanna Reitz
` (4 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:27 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
In order to let qemu-img info present a block graph, add a parameter to
bdrv_node_info_dump() and bdrv_image_info_specific_dump() so that the
information of nodes below the root level can be given an indentation.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
include/block/qapi.h | 5 ++--
block/monitor/block-hmp-cmds.c | 2 +-
block/qapi.c | 47 +++++++++++++++++++---------------
qemu-img.c | 2 +-
qemu-io-cmds.c | 3 ++-
5 files changed, 34 insertions(+), 25 deletions(-)
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 196436020e..38855f2ae9 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -49,6 +49,7 @@ void bdrv_query_block_graph_info(BlockDriverState *bs,
void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
- const char *prefix);
-void bdrv_node_info_dump(BlockNodeInfo *info);
+ const char *prefix,
+ int indentation);
+void bdrv_node_info_dump(BlockNodeInfo *info, int indentation);
#endif
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d286f2d1c5..a63c25c208 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -734,7 +734,7 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
monitor_printf(mon, "\nImages:\n");
image_info = inserted->image;
while (1) {
- bdrv_node_info_dump(qapi_ImageInfo_base(image_info));
+ bdrv_node_info_dump(qapi_ImageInfo_base(image_info), 0);
if (image_info->has_backing_image) {
image_info = image_info->backing_image;
} else {
diff --git a/block/qapi.c b/block/qapi.c
index f208c21ccf..3e35603f0c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -915,7 +915,8 @@ static bool qobject_is_empty_dump(const QObject *obj)
* prepending an optional prefix if the dump is not empty.
*/
void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
- const char *prefix)
+ const char *prefix,
+ int indentation)
{
QObject *obj, *data;
Visitor *v = qobject_output_visitor_new(&obj);
@@ -925,48 +926,51 @@ void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
data = qdict_get(qobject_to(QDict, obj), "data");
if (!qobject_is_empty_dump(data)) {
if (prefix) {
- qemu_printf("%s", prefix);
+ qemu_printf("%*s%s", indentation * 4, "", prefix);
}
- dump_qobject(1, data);
+ dump_qobject(indentation + 1, data);
}
qobject_unref(obj);
visit_free(v);
}
-void bdrv_node_info_dump(BlockNodeInfo *info)
+void bdrv_node_info_dump(BlockNodeInfo *info, int indentation)
{
char *size_buf, *dsize_buf;
+ g_autofree char *ind_s = g_strdup_printf("%*s", indentation * 4, "");
+
if (!info->has_actual_size) {
dsize_buf = g_strdup("unavailable");
} else {
dsize_buf = size_to_str(info->actual_size);
}
size_buf = size_to_str(info->virtual_size);
- qemu_printf("image: %s\n"
- "file format: %s\n"
- "virtual size: %s (%" PRId64 " bytes)\n"
- "disk size: %s\n",
- info->filename, info->format, size_buf,
- info->virtual_size,
- dsize_buf);
+ qemu_printf("%simage: %s\n"
+ "%sfile format: %s\n"
+ "%svirtual size: %s (%" PRId64 " bytes)\n"
+ "%sdisk size: %s\n",
+ ind_s, info->filename,
+ ind_s, info->format,
+ ind_s, size_buf, info->virtual_size,
+ ind_s, dsize_buf);
g_free(size_buf);
g_free(dsize_buf);
if (info->has_encrypted && info->encrypted) {
- qemu_printf("encrypted: yes\n");
+ qemu_printf("%sencrypted: yes\n", ind_s);
}
if (info->has_cluster_size) {
- qemu_printf("cluster_size: %" PRId64 "\n",
- info->cluster_size);
+ qemu_printf("%scluster_size: %" PRId64 "\n",
+ ind_s, info->cluster_size);
}
if (info->has_dirty_flag && info->dirty_flag) {
- qemu_printf("cleanly shut down: no\n");
+ qemu_printf("%scleanly shut down: no\n", ind_s);
}
if (info->has_backing_filename) {
- qemu_printf("backing file: %s", info->backing_filename);
+ qemu_printf("%sbacking file: %s", ind_s, info->backing_filename);
if (!info->has_full_backing_filename) {
qemu_printf(" (cannot determine actual path)");
} else if (strcmp(info->backing_filename,
@@ -975,15 +979,16 @@ void bdrv_node_info_dump(BlockNodeInfo *info)
}
qemu_printf("\n");
if (info->has_backing_filename_format) {
- qemu_printf("backing file format: %s\n",
- info->backing_filename_format);
+ qemu_printf("%sbacking file format: %s\n",
+ ind_s, info->backing_filename_format);
}
}
if (info->has_snapshots) {
SnapshotInfoList *elem;
- qemu_printf("Snapshot list:\n");
+ qemu_printf("%sSnapshot list:\n", ind_s);
+ qemu_printf("%s", ind_s);
bdrv_snapshot_dump(NULL);
qemu_printf("\n");
@@ -1003,6 +1008,7 @@ void bdrv_node_info_dump(BlockNodeInfo *info)
pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+ qemu_printf("%s", ind_s);
bdrv_snapshot_dump(&sn);
qemu_printf("\n");
}
@@ -1010,6 +1016,7 @@ void bdrv_node_info_dump(BlockNodeInfo *info)
if (info->has_format_specific) {
bdrv_image_info_specific_dump(info->format_specific,
- "Format specific information:\n");
+ "Format specific information:\n",
+ indentation);
}
}
diff --git a/qemu-img.c b/qemu-img.c
index 3b1f950c85..f835ce6feb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2843,7 +2843,7 @@ static void dump_human_image_info_list(BlockNodeInfoList *list)
}
delim = true;
- bdrv_node_info_dump(elem->value);
+ bdrv_node_info_dump(elem->value, 0);
}
}
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 084ec44d3b..00f1f78cdd 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1820,7 +1820,8 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
}
if (spec_info) {
bdrv_image_info_specific_dump(spec_info,
- "Format specific information:\n");
+ "Format specific information:\n",
+ 0);
qapi_free_ImageInfoSpecific(spec_info);
}
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 09/12] iotests: Filter child node information
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (7 preceding siblings ...)
2022-06-20 16:27 ` [PATCH v2 08/12] block/qapi: Add indentation to bdrv_node_info_dump() Hanna Reitz
@ 2022-06-20 16:27 ` Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 10/12] iotests/106, 214, 308: Read only one size line Hanna Reitz
` (3 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:27 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
Before we let qemu-img info print child node information, have
common.filter, common.rc, and iotests.py filter it from the test output
so we get as few reference output changes as possible.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
tests/qemu-iotests/common.filter | 22 ++++++++++++++--------
tests/qemu-iotests/common.rc | 22 ++++++++++++++--------
tests/qemu-iotests/iotests.py | 18 +++++++++++++++---
3 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index cc9f1a5891..6b32c7fbfa 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -223,6 +223,7 @@ _filter_img_info()
discard=0
regex_json_spec_start='^ *"format-specific": \{'
+ regex_json_child_start='^ *"children": \['
gsed -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
-e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
-e "s#$TEST_DIR#TEST_DIR#g" \
@@ -251,20 +252,25 @@ _filter_img_info()
-e 's/\(compression type: \)\(zlib\|zstd\)/\1COMPRESSION_TYPE/' \
-e "s/uuid: [-a-f0-9]\\+/uuid: 00000000-0000-0000-0000-000000000000/" | \
while IFS='' read -r line; do
- if [[ $format_specific == 1 ]]; then
- discard=0
- elif [[ $line == "Format specific information:" ]]; then
- discard=1
- elif [[ $line =~ $regex_json_spec_start ]]; then
- discard=2
- regex_json_spec_end="^${line%%[^ ]*}\\},? *$"
+ if [[ $discard == 0 ]]; then
+ if [[ $format_specific == 0 && $line == "Format specific information:" ]]; then
+ discard=1
+ elif [[ $line =~ "Child node '/" ]]; then
+ discard=1
+ elif [[ $line =~ $regex_json_spec_start ]]; then
+ discard=2
+ regex_json_end="^${line%%[^ ]*}\\},? *$"
+ elif [[ $line =~ $regex_json_child_start ]]; then
+ discard=2
+ regex_json_end="^${line%%[^ ]*}\\],? *$"
+ fi
fi
if [[ $discard == 0 ]]; then
echo "$line"
elif [[ $discard == 1 && ! $line ]]; then
echo
discard=0
- elif [[ $discard == 2 && $line =~ $regex_json_spec_end ]]; then
+ elif [[ $discard == 2 && $line =~ $regex_json_end ]]; then
discard=0
fi
done
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 165b54a61e..0e1fa82ec6 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -711,6 +711,7 @@ _img_info()
discard=0
regex_json_spec_start='^ *"format-specific": \{'
+ regex_json_child_start='^ *"children": \['
$QEMU_IMG info $QEMU_IMG_EXTRA_ARGS "$@" "$TEST_IMG" 2>&1 | \
sed -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
-e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
@@ -721,20 +722,25 @@ _img_info()
-e "/^disk size:/ D" \
-e "/actual-size/ D" | \
while IFS='' read -r line; do
- if [[ $format_specific == 1 ]]; then
- discard=0
- elif [[ $line == "Format specific information:" ]]; then
- discard=1
- elif [[ $line =~ $regex_json_spec_start ]]; then
- discard=2
- regex_json_spec_end="^${line%%[^ ]*}\\},? *$"
+ if [[ $discard == 0 ]]; then
+ if [[ $format_specific == 0 && $line == "Format specific information:" ]]; then
+ discard=1
+ elif [[ $line =~ "Child node '/" ]]; then
+ discard=1
+ elif [[ $format_specific == 0 && $line =~ $regex_json_spec_start ]]; then
+ discard=2
+ regex_json_end="^${line%%[^ ]*}\\},? *$"
+ elif [[ $line =~ $regex_json_child_start ]]; then
+ discard=2
+ regex_json_end="^${line%%[^ ]*}\\],? *$"
+ fi
fi
if [[ $discard == 0 ]]; then
echo "$line"
elif [[ $discard == 1 && ! $line ]]; then
echo
discard=0
- elif [[ $discard == 2 && $line =~ $regex_json_spec_end ]]; then
+ elif [[ $discard == 2 && $line =~ $regex_json_end ]]; then
discard=0
fi
done
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index da7d6637e1..94aeb3f3b2 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -329,7 +329,7 @@ def qemu_img_log(*args: str, check: bool = True
def img_info_log(filename: str, filter_path: Optional[str] = None,
use_image_opts: bool = False, extra_args: Sequence[str] = (),
- check: bool = True,
+ check: bool = True, drop_child_info: bool = True,
) -> None:
args = ['info']
if use_image_opts:
@@ -342,7 +342,7 @@ def img_info_log(filename: str, filter_path: Optional[str] = None,
output = qemu_img(*args, check=check).stdout
if not filter_path:
filter_path = filename
- log(filter_img_info(output, filter_path))
+ log(filter_img_info(output, filter_path, drop_child_info))
def qemu_io_wrap_args(args: Sequence[str]) -> List[str]:
if '-f' in args or '--image-opts' in args:
@@ -642,11 +642,23 @@ def _filter(_key, value):
def filter_generated_node_ids(msg):
return re.sub("#block[0-9]+", "NODE_NAME", msg)
-def filter_img_info(output, filename):
+def filter_img_info(output: str, filename: str,
+ drop_child_info: bool = True) -> str:
lines = []
+ drop_indented = False
for line in output.split('\n'):
if 'disk size' in line or 'actual-size' in line:
continue
+
+ # Drop child node info
+ if drop_indented:
+ if line.startswith(' '):
+ continue
+ drop_indented = False
+ if drop_child_info and "Child node '/" in line:
+ drop_indented = True
+ continue
+
line = line.replace(filename, 'TEST_IMG')
line = filter_testfiles(line)
line = line.replace(imgfmt, 'IMGFMT')
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 10/12] iotests/106, 214, 308: Read only one size line
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (8 preceding siblings ...)
2022-06-20 16:27 ` [PATCH v2 09/12] iotests: Filter child node information Hanna Reitz
@ 2022-06-20 16:27 ` Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 11/12] qemu-img: Let info print block graph Hanna Reitz
` (2 subsequent siblings)
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:27 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
These tests read size information (sometimes disk size, sometimes
virtual size) from qemu-img info's output. Once qemu-img starts
printing info about child nodes, we are going to see multiple instances
of that per image, but these tests are only interested in the first one,
so use "head -n 1" to get it.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
tests/qemu-iotests/106 | 4 ++--
tests/qemu-iotests/214 | 6 ++++--
tests/qemu-iotests/308 | 4 ++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
index 9d6adb542d..ae0fc46691 100755
--- a/tests/qemu-iotests/106
+++ b/tests/qemu-iotests/106
@@ -66,7 +66,7 @@ for create_mode in off falloc full; do
expected_size=$((expected_size + $GROWTH_SIZE))
fi
- actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
+ actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size' | head -n 1)
actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
# The actual size may exceed the expected size, depending on the file
@@ -105,7 +105,7 @@ for growth_mode in falloc full; do
_make_test_img -o "extent_size_hint=0" 2G
$QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG" +${GROWTH_SIZE}K
- actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
+ actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size' | head -n 1)
actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
if [ $actual_size -lt $GROWTH_SIZE ]; then
diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index c66e246ba2..55ffcd7f44 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -102,7 +102,8 @@ let data_size="8 * $cluster_size"
$QEMU_IO -c "write -P 0xaa 0 $data_size" "$TEST_IMG" \
2>&1 | _filter_qemu_io | _filter_testdir
sizeA=$($QEMU_IMG info --output=json "$TEST_IMG" |
- sed -n '/"actual-size":/ s/[^0-9]//gp')
+ sed -n '/"actual-size":/ s/[^0-9]//gp' |
+ head -n 1)
_make_test_img 2M -o cluster_size=$cluster_size
echo "Write compressed data:"
@@ -124,7 +125,8 @@ $QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
_filter_qemu_io | _filter_testdir
sizeB=$($QEMU_IMG info --output=json "$TEST_IMG" |
- sed -n '/"actual-size":/ s/[^0-9]//gp')
+ sed -n '/"actual-size":/ s/[^0-9]//gp' |
+ head -n 1)
if [ $sizeA -lt $sizeB ]
then
diff --git a/tests/qemu-iotests/308 b/tests/qemu-iotests/308
index bde4aac2fa..09275e9a10 100755
--- a/tests/qemu-iotests/308
+++ b/tests/qemu-iotests/308
@@ -217,12 +217,12 @@ echo
echo '=== Remove export ==='
# Double-check that $EXT_MP appears as a non-empty file (the raw image)
-$QEMU_IMG info -f raw "$EXT_MP" | grep 'virtual size'
+$QEMU_IMG info -f raw "$EXT_MP" | grep 'virtual size' | head -n 1
fuse_export_del 'export-mp'
# See that the file appears empty again
-$QEMU_IMG info -f raw "$EXT_MP" | grep 'virtual size'
+$QEMU_IMG info -f raw "$EXT_MP" | grep 'virtual size' | head -n 1
echo
echo '=== Writable export ==='
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 11/12] qemu-img: Let info print block graph
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (9 preceding siblings ...)
2022-06-20 16:27 ` [PATCH v2 10/12] iotests/106, 214, 308: Read only one size line Hanna Reitz
@ 2022-06-20 16:27 ` Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 12/12] qemu-img: Change info key names for protocol nodes Hanna Reitz
2022-12-08 12:24 ` [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:27 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
For every node in the backing chain, collect its BlockGraphInfo struct
using bdrv_query_block_graph_info(). Print all nodes' information,
indenting child nodes and labelling them with a path constructed from
the child names leading to the node from the root (e.g. /file/file).
Note that we open each image with BDRV_O_NO_BACKING, so its backing
child is omitted from this graph, and thus presented in the previous
manner: By simply concatenating all images' information, separated with
blank lines.
This affects two iotests:
- 065: Here we try to get the format node's format specific information.
The pre-patch code does so by taking all lines from "Format specific
information:" until an empty line. This format specific information
is no longer followed by an empty line, though, but by child node
information, so limit the range by "Child node '/file':".
- 302: Calls qemu_img() for qemu-img info directly, which does not
filter the output, so the child node information ends up in the
output.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
qapi/block-core.json | 4 +--
qemu-img.c | 69 ++++++++++++++++++++++++++------------
tests/qemu-iotests/065 | 2 +-
tests/qemu-iotests/302.out | 5 +++
4 files changed, 56 insertions(+), 24 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b7e5708487..5597c783e7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5715,9 +5715,9 @@
##
# @DummyBlockCoreForceArrays:
#
-# Not used by QMP; hack to let us use BlockNodeInfoList internally
+# Not used by QMP; hack to let us use BlockGraphInfoList internally
#
# Since: 7.1
##
{ 'struct': 'DummyBlockCoreForceArrays',
- 'data': { 'unused-block-node-info': ['BlockNodeInfo'] } }
+ 'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
diff --git a/qemu-img.c b/qemu-img.c
index f835ce6feb..6da4064d57 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2800,13 +2800,13 @@ static void dump_snapshots(BlockDriverState *bs)
g_free(sn_tab);
}
-static void dump_json_block_node_info_list(BlockNodeInfoList *list)
+static void dump_json_block_graph_info_list(BlockGraphInfoList *list)
{
GString *str;
QObject *obj;
Visitor *v = qobject_output_visitor_new(&obj);
- visit_type_BlockNodeInfoList(v, NULL, &list, &error_abort);
+ visit_type_BlockGraphInfoList(v, NULL, &list, &error_abort);
visit_complete(v, &obj);
str = qobject_to_json_pretty(obj, true);
assert(str != NULL);
@@ -2816,13 +2816,13 @@ static void dump_json_block_node_info_list(BlockNodeInfoList *list)
g_string_free(str, true);
}
-static void dump_json_block_node_info(BlockNodeInfo *info)
+static void dump_json_block_graph_info(BlockGraphInfo *info)
{
GString *str;
QObject *obj;
Visitor *v = qobject_output_visitor_new(&obj);
- visit_type_BlockNodeInfo(v, NULL, &info, &error_abort);
+ visit_type_BlockGraphInfo(v, NULL, &info, &error_abort);
visit_complete(v, &obj);
str = qobject_to_json_pretty(obj, true);
assert(str != NULL);
@@ -2832,9 +2832,29 @@ static void dump_json_block_node_info(BlockNodeInfo *info)
g_string_free(str, true);
}
-static void dump_human_image_info_list(BlockNodeInfoList *list)
+static void dump_human_image_info(BlockGraphInfo *info, int indentation,
+ const char *path)
{
- BlockNodeInfoList *elem;
+ BlockChildInfoList *children_list;
+
+ bdrv_node_info_dump(qapi_BlockGraphInfo_base(info), indentation);
+
+ for (children_list = info->children; children_list;
+ children_list = children_list->next)
+ {
+ BlockChildInfo *child = children_list->value;
+ g_autofree char *child_path;
+
+ printf("%*sChild node '%s%s':\n",
+ indentation * 4, "", path, child->name);
+ child_path = g_strdup_printf("%s%s/", path, child->name);
+ dump_human_image_info(child->info, indentation + 1, child_path);
+ }
+}
+
+static void dump_human_image_info_list(BlockGraphInfoList *list)
+{
+ BlockGraphInfoList *elem;
bool delim = false;
for (elem = list; elem; elem = elem->next) {
@@ -2843,7 +2863,7 @@ static void dump_human_image_info_list(BlockNodeInfoList *list)
}
delim = true;
- bdrv_node_info_dump(elem->value, 0);
+ dump_human_image_info(elem->value, 0, "/");
}
}
@@ -2853,7 +2873,7 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
}
/**
- * Open an image file chain and return an BlockNodeInfoList
+ * Open an image file chain and return an BlockGraphInfoList
*
* @filename: topmost image filename
* @fmt: topmost image format (may be NULL to autodetect)
@@ -2864,13 +2884,13 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
* opening an image file. If there was an error a message will have been
* printed to stderr.
*/
-static BlockNodeInfoList *collect_image_info_list(bool image_opts,
- const char *filename,
- const char *fmt,
- bool chain, bool force_share)
+static BlockGraphInfoList *collect_image_info_list(bool image_opts,
+ const char *filename,
+ const char *fmt,
+ bool chain, bool force_share)
{
- BlockNodeInfoList *head = NULL;
- BlockNodeInfoList **tail = &head;
+ BlockGraphInfoList *head = NULL;
+ BlockGraphInfoList **tail = &head;
GHashTable *filenames;
Error *err = NULL;
@@ -2879,7 +2899,7 @@ static BlockNodeInfoList *collect_image_info_list(bool image_opts,
while (filename) {
BlockBackend *blk;
BlockDriverState *bs;
- BlockNodeInfo *info;
+ BlockGraphInfo *info;
if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
error_report("Backing file '%s' creates an infinite loop.",
@@ -2896,7 +2916,14 @@ static BlockNodeInfoList *collect_image_info_list(bool image_opts,
}
bs = blk_bs(blk);
- bdrv_query_block_node_info(bs, &info, &err);
+ /*
+ * Note that the returned BlockGraphInfo object will not have
+ * information about this image's backing node, because we have opened
+ * it with BDRV_O_NO_BACKING. Printing this object will therefore not
+ * duplicate the backing chain information that we obtain by walking
+ * the chain manually here.
+ */
+ bdrv_query_block_graph_info(bs, &info, &err);
if (err) {
error_report_err(err);
blk_unref(blk);
@@ -2929,7 +2956,7 @@ static BlockNodeInfoList *collect_image_info_list(bool image_opts,
return head;
err:
- qapi_free_BlockNodeInfoList(head);
+ qapi_free_BlockGraphInfoList(head);
g_hash_table_destroy(filenames);
return NULL;
}
@@ -2940,7 +2967,7 @@ static int img_info(int argc, char **argv)
OutputFormat output_format = OFORMAT_HUMAN;
bool chain = false;
const char *filename, *fmt, *output;
- BlockNodeInfoList *list;
+ BlockGraphInfoList *list;
bool image_opts = false;
bool force_share = false;
@@ -3019,14 +3046,14 @@ static int img_info(int argc, char **argv)
break;
case OFORMAT_JSON:
if (chain) {
- dump_json_block_node_info_list(list);
+ dump_json_block_graph_info_list(list);
} else {
- dump_json_block_node_info(list->value);
+ dump_json_block_graph_info(list->value);
}
break;
}
- qapi_free_BlockNodeInfoList(list);
+ qapi_free_BlockGraphInfoList(list);
return 0;
}
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index b724c89c7c..b76701c71e 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -56,7 +56,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
def test_human(self):
data = qemu_img('info', '--output=human', test_img).stdout.split('\n')
data = data[(data.index('Format specific information:') + 1)
- :data.index('')]
+ :data.index("Child node '/file':")]
for field in data:
self.assertTrue(re.match('^ {4}[^ ]', field) is not None)
data = [line.strip() for line in data]
diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
index 3e7c281b91..edfa1c4f05 100644
--- a/tests/qemu-iotests/302.out
+++ b/tests/qemu-iotests/302.out
@@ -4,6 +4,11 @@ image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
file format: raw
virtual size: 448 KiB (458752 bytes)
disk size: unavailable
+Child node '/file':
+ image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
+ file format: nbd
+ virtual size: 448 KiB (458752 bytes)
+ disk size: unavailable
=== Converted image info ===
image: TEST_IMG
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 12/12] qemu-img: Change info key names for protocol nodes
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (10 preceding siblings ...)
2022-06-20 16:27 ` [PATCH v2 11/12] qemu-img: Let info print block graph Hanna Reitz
@ 2022-06-20 16:27 ` Hanna Reitz
2022-12-08 12:24 ` [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
12 siblings, 0 replies; 24+ messages in thread
From: Hanna Reitz @ 2022-06-20 16:27 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Hanna Reitz, Kevin Wolf, Eric Blake, Markus Armbruster
Currently, when querying a qcow2 image, qemu-img info reports something
like this:
image: test.qcow2
file format: qcow2
virtual size: 64 MiB (67108864 bytes)
disk size: 196 KiB
cluster_size: 65536
Format specific information:
compat: 1.1
compression type: zlib
lazy refcounts: false
refcount bits: 16
corrupt: false
extended l2: false
Child node '/file':
image: test.qcow2
file format: file
virtual size: 192 KiB (197120 bytes)
disk size: 196 KiB
Format specific information:
extent size hint: 1048576
Notably, the way the keys are named is specific for image files: The
filename is shown under "image", the BDS driver under "file format", and
the BDS length under "virtual size". This does not make much sense for
nodes that are not actually supposed to be guest images, like the /file
child node shown above.
Give bdrv_node_info_dump() a @protocol parameter that gives a hint that
the respective node is probably just used for data storage and does not
necessarily present the data for a VM guest disk. This renames the keys
so that with this patch, the output becomes:
image: test.qcow2
[...]
Child node '/file':
filename: test.qcow2
protocol type: file
file length: 192 KiB (197120 bytes)
disk size: 196 KiB
Format specific information:
extent size hint: 1048576
(Perhaps we should also rename "Format specific information", but I
could not come up with anything better that will not become problematic
if we guess wrong with the protocol "heuristic".)
This change affects iotest 302, which has protocol node information in
its reference output.
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
This patch is completely optional and just based on personal
preference. I just don't like e.g. how the child node is called an
"image" when it's pretty much unusable as a guest image, or how "virtual
size" is there twice but with different values.
---
include/block/qapi.h | 2 +-
block/monitor/block-hmp-cmds.c | 2 +-
block/qapi.c | 39 ++++++++++++++++++++++++++++------
qemu-img.c | 3 ++-
tests/qemu-iotests/302.out | 6 +++---
5 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 38855f2ae9..26113da21a 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -51,5 +51,5 @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
const char *prefix,
int indentation);
-void bdrv_node_info_dump(BlockNodeInfo *info, int indentation);
+void bdrv_node_info_dump(BlockNodeInfo *info, int indentation, bool protocol);
#endif
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index a63c25c208..a8067846b2 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -734,7 +734,7 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
monitor_printf(mon, "\nImages:\n");
image_info = inserted->image;
while (1) {
- bdrv_node_info_dump(qapi_ImageInfo_base(image_info), 0);
+ bdrv_node_info_dump(qapi_ImageInfo_base(image_info), 0, false);
if (image_info->has_backing_image) {
image_info = image_info->backing_image;
} else {
diff --git a/block/qapi.c b/block/qapi.c
index 3e35603f0c..56f398c500 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -934,24 +934,49 @@ void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
visit_free(v);
}
-void bdrv_node_info_dump(BlockNodeInfo *info, int indentation)
+/**
+ * Print the given @info object in human-readable form. Every field is indented
+ * using the given @indentation (four spaces per indentation level).
+ *
+ * When using this to print a whole block graph, @protocol can be set to true to
+ * signify that the given information is associated with a protocol node, i.e.
+ * just data storage for an image, such that the data it presents is not really
+ * a full VM disk. If so, several fields change name: For example, "virtual
+ * size" is printed as "file length".
+ * (Consider a qcow2 image, which is represented by a qcow2 node and a file
+ * node. Printing a "virtual size" for the file node does not make sense,
+ * because without the qcow2 node, it is not really a guest disk, so it does not
+ * have a "virtual size". Therefore, we call it "file length" instead.)
+ *
+ * @protocol is ignored when @indentation is 0, because we take that to mean
+ * that the associated node is the root node in the queried block graph, and
+ * thus is always to be interpreted as a standalone guest disk.
+ */
+void bdrv_node_info_dump(BlockNodeInfo *info, int indentation, bool protocol)
{
char *size_buf, *dsize_buf;
g_autofree char *ind_s = g_strdup_printf("%*s", indentation * 4, "");
+ if (indentation == 0) {
+ /* Top level, consider this a normal image */
+ protocol = false;
+ }
+
if (!info->has_actual_size) {
dsize_buf = g_strdup("unavailable");
} else {
dsize_buf = size_to_str(info->actual_size);
}
size_buf = size_to_str(info->virtual_size);
- qemu_printf("%simage: %s\n"
- "%sfile format: %s\n"
- "%svirtual size: %s (%" PRId64 " bytes)\n"
+ qemu_printf("%s%s: %s\n"
+ "%s%s: %s\n"
+ "%s%s: %s (%" PRId64 " bytes)\n"
"%sdisk size: %s\n",
- ind_s, info->filename,
- ind_s, info->format,
- ind_s, size_buf, info->virtual_size,
+ ind_s, protocol ? "filename" : "image", info->filename,
+ ind_s, protocol ? "protocol type" : "file format",
+ info->format,
+ ind_s, protocol ? "file length" : "virtual size",
+ size_buf, info->virtual_size,
ind_s, dsize_buf);
g_free(size_buf);
g_free(dsize_buf);
diff --git a/qemu-img.c b/qemu-img.c
index 6da4064d57..17ccde5448 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2837,7 +2837,8 @@ static void dump_human_image_info(BlockGraphInfo *info, int indentation,
{
BlockChildInfoList *children_list;
- bdrv_node_info_dump(qapi_BlockGraphInfo_base(info), indentation);
+ bdrv_node_info_dump(qapi_BlockGraphInfo_base(info), indentation,
+ info->children == NULL);
for (children_list = info->children; children_list;
children_list = children_list->next)
diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
index edfa1c4f05..7b5014cdd8 100644
--- a/tests/qemu-iotests/302.out
+++ b/tests/qemu-iotests/302.out
@@ -5,9 +5,9 @@ file format: raw
virtual size: 448 KiB (458752 bytes)
disk size: unavailable
Child node '/file':
- image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
- file format: nbd
- virtual size: 448 KiB (458752 bytes)
+ filename: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
+ protocol type: nbd
+ file length: 448 KiB (458752 bytes)
disk size: unavailable
=== Converted image info ===
--
2.35.3
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/12] qemu-img info: Show protocol-level information
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
` (11 preceding siblings ...)
2022-06-20 16:27 ` [PATCH v2 12/12] qemu-img: Change info key names for protocol nodes Hanna Reitz
@ 2022-12-08 12:24 ` Hanna Reitz
2023-01-19 20:12 ` Kevin Wolf
12 siblings, 1 reply; 24+ messages in thread
From: Hanna Reitz @ 2022-12-08 12:24 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, Markus Armbruster
On 20.06.22 18:26, Hanna Reitz wrote:
> Hi,
>
> This series is a v2 to:
>
> https://lists.nongnu.org/archive/html/qemu-block/2022-05/msg00042.html
Ping, it looks like this still applies (to the master branch and kevin’s
block-next branch at least).
Hanna
> So the final state is that despite the QAPI changes, hopefully only the
> qemu-img info output changes, and it now prints every image node’s
> subgraph. This results in an output like the following (for a qcow2
> image):
>
> image: test.qcow2
> file format: qcow2
> virtual size: 64 MiB (67108864 bytes)
> disk size: 196 KiB
> cluster_size: 65536
> Format specific information:
> compat: 1.1
> compression type: zlib
> lazy refcounts: false
> refcount bits: 16
> corrupt: false
> extended l2: false
> Child node '/file':
> filename: test.qcow2
> protocol type: file
> file length: 192 KiB (197120 bytes)
> disk size: 196 KiB
> Format specific information:
> extent size hint: 1048576
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 01/12] block: Improve empty format-specific info dump
2022-06-20 16:26 ` [PATCH v2 01/12] block: Improve empty format-specific info dump Hanna Reitz
@ 2023-01-19 14:00 ` Kevin Wolf
2023-01-20 13:35 ` Hanna Czenczek
0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2023-01-19 14:00 UTC (permalink / raw)
To: Hanna Reitz; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> When a block driver supports obtaining format-specific information, but
> that object only contains optional fields, it is possible that none of
> them are present, so that dump_qobject() (called by
> bdrv_image_info_specific_dump()) will not print anything.
>
> The callers of bdrv_image_info_specific_dump() put a header above this
> information ("Format specific information:\n"), which will look strange
> when there is nothing below. Modify bdrv_image_info_specific_dump() to
> print this header instead of its callers, and only if there is indeed
> something to be printed.
>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 2f0d8ac25a..084ec44d3b 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1819,8 +1819,8 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
> return -EIO;
> }
> if (spec_info) {
> - printf("Format specific information:\n");
> - bdrv_image_info_specific_dump(spec_info);
> + bdrv_image_info_specific_dump(spec_info,
> + "Format specific information:\n");
> qapi_free_ImageInfoSpecific(spec_info);
> }
Interesting observation here: That qemu-io uses printf() instead of
qemu_printf() for the top level, but then dump_qobject() (which results
in qemu_printf()) for the format specific information, means that if you
use the 'qemu-io' HMP command, you'll get half of the output on stdout
and the other half in the monitor.
This series doesn't fix this, but the split makes a little more sense
after this patch at least...
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 03/12] block/vmdk: Change extent info type
2022-06-20 16:26 ` [PATCH v2 03/12] block/vmdk: Change extent info type Hanna Reitz
@ 2023-01-19 15:20 ` Kevin Wolf
2023-01-20 13:43 ` Hanna Czenczek
2023-01-27 23:06 ` Eric Blake
0 siblings, 2 replies; 24+ messages in thread
From: Kevin Wolf @ 2023-01-19 15:20 UTC (permalink / raw)
To: Hanna Reitz; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> VMDK's implementation of .bdrv_get_specific_info() returns information
> about its extent files, ostensibly in the form of ImageInfo objects.
> However, it does not get this information through
> bdrv_query_image_info(), but fills only a select few fields with custom
> information that does not always match the fields' purposes.
>
> For example, @format, which is supposed to be a block driver name, is
> filled with the extent type, e.g. SPARSE or FLAT.
>
> In ImageInfo, @compressed shows whether the data that can be seen in the
> image is stored in compressed form or not. For example, a compressed
> qcow2 image will store compressed data in its data file, but when
> accessing the qcow2 node, you will see normal data. This is not how
> VMDK uses the @compressed field for its extent files: Instead, it
> signifies whether accessing the extent file will yield compressed data
> (which the VMDK driver then (de-)compresses).
Actually, VMDK was the only user of the field in ImageInfo. qcow2
doesn't set the field at all because it would have to parse all L2
tables in order to set it.
So I suppose @compressed can now be removed from ImageInfo?
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/12] qemu-img info: Show protocol-level information
2022-12-08 12:24 ` [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
@ 2023-01-19 20:12 ` Kevin Wolf
2023-01-20 13:44 ` Hanna Czenczek
0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2023-01-19 20:12 UTC (permalink / raw)
To: Hanna Reitz; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
Am 08.12.2022 um 13:24 hat Hanna Reitz geschrieben:
> On 20.06.22 18:26, Hanna Reitz wrote:
> > Hi,
> >
> > This series is a v2 to:
> >
> > https://lists.nongnu.org/archive/html/qemu-block/2022-05/msg00042.html
>
> Ping, it looks like this still applies (to the master branch and kevin’s
> block-next branch at least).
Not any more. :-)
But the conflicts seemed obvious enough, so I rebased it (including
changing the "Since: 7.1" occurrences to 8.0) and applied it to my block
branch. Thanks!
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 01/12] block: Improve empty format-specific info dump
2023-01-19 14:00 ` Kevin Wolf
@ 2023-01-20 13:35 ` Hanna Czenczek
2023-01-20 14:08 ` Kevin Wolf
0 siblings, 1 reply; 24+ messages in thread
From: Hanna Czenczek @ 2023-01-20 13:35 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
On 19.01.23 15:00, Kevin Wolf wrote:
> Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
>> When a block driver supports obtaining format-specific information, but
>> that object only contains optional fields, it is possible that none of
>> them are present, so that dump_qobject() (called by
>> bdrv_image_info_specific_dump()) will not print anything.
>>
>> The callers of bdrv_image_info_specific_dump() put a header above this
>> information ("Format specific information:\n"), which will look strange
>> when there is nothing below. Modify bdrv_image_info_specific_dump() to
>> print this header instead of its callers, and only if there is indeed
>> something to be printed.
>>
>> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>> index 2f0d8ac25a..084ec44d3b 100644
>> --- a/qemu-io-cmds.c
>> +++ b/qemu-io-cmds.c
>> @@ -1819,8 +1819,8 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
>> return -EIO;
>> }
>> if (spec_info) {
>> - printf("Format specific information:\n");
>> - bdrv_image_info_specific_dump(spec_info);
>> + bdrv_image_info_specific_dump(spec_info,
>> + "Format specific information:\n");
>> qapi_free_ImageInfoSpecific(spec_info);
>> }
> Interesting observation here: That qemu-io uses printf() instead of
> qemu_printf() for the top level, but then dump_qobject() (which results
> in qemu_printf()) for the format specific information, means that if you
> use the 'qemu-io' HMP command, you'll get half of the output on stdout
> and the other half in the monitor.
Hu. I can’t find a single instance of qemu_printf() in qemu-io-cmds.c,
but then I assume all printf()s there should really be qemu_printf()?
Hanna
> This series doesn't fix this, but the split makes a little more sense
> after this patch at least...
>
> Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 03/12] block/vmdk: Change extent info type
2023-01-19 15:20 ` Kevin Wolf
@ 2023-01-20 13:43 ` Hanna Czenczek
2023-01-27 23:06 ` Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: Hanna Czenczek @ 2023-01-20 13:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
On 19.01.23 16:20, Kevin Wolf wrote:
> Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
>> VMDK's implementation of .bdrv_get_specific_info() returns information
>> about its extent files, ostensibly in the form of ImageInfo objects.
>> However, it does not get this information through
>> bdrv_query_image_info(), but fills only a select few fields with custom
>> information that does not always match the fields' purposes.
>>
>> For example, @format, which is supposed to be a block driver name, is
>> filled with the extent type, e.g. SPARSE or FLAT.
>>
>> In ImageInfo, @compressed shows whether the data that can be seen in the
>> image is stored in compressed form or not. For example, a compressed
>> qcow2 image will store compressed data in its data file, but when
>> accessing the qcow2 node, you will see normal data. This is not how
>> VMDK uses the @compressed field for its extent files: Instead, it
>> signifies whether accessing the extent file will yield compressed data
>> (which the VMDK driver then (de-)compresses).
> Actually, VMDK was the only user of the field in ImageInfo. qcow2
> doesn't set the field at all because it would have to parse all L2
> tables in order to set it.
Right, makes sense.
> So I suppose @compressed can now be removed from ImageInfo?
I think so. Looks like it was indeed introduced specifically for VMDK’s extent information (cbe82d7fb32, f4c129a38a5) so that ImageInfo could be used there, which this patch here changes. So we should probably indeed remove the field – it did lead me to naïvely believe that it would have a meaning on images that actually support compression (like qcow2) after all.
Hanna
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/12] qemu-img info: Show protocol-level information
2023-01-19 20:12 ` Kevin Wolf
@ 2023-01-20 13:44 ` Hanna Czenczek
0 siblings, 0 replies; 24+ messages in thread
From: Hanna Czenczek @ 2023-01-20 13:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
On 19.01.23 21:12, Kevin Wolf wrote:
> Am 08.12.2022 um 13:24 hat Hanna Reitz geschrieben:
>> On 20.06.22 18:26, Hanna Reitz wrote:
>>> Hi,
>>>
>>> This series is a v2 to:
>>>
>>> https://lists.nongnu.org/archive/html/qemu-block/2022-05/msg00042.html
>> Ping, it looks like this still applies (to the master branch and kevin’s
>> block-next branch at least).
> Not any more. :-)
>
> But the conflicts seemed obvious enough, so I rebased it (including
> changing the "Since: 7.1" occurrences to 8.0) and applied it to my block
> branch.
Ah, yes. That I should have fixed.
> Thanks!
Thank you! :)
Hanna
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 01/12] block: Improve empty format-specific info dump
2023-01-20 13:35 ` Hanna Czenczek
@ 2023-01-20 14:08 ` Kevin Wolf
0 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2023-01-20 14:08 UTC (permalink / raw)
To: Hanna Czenczek; +Cc: qemu-block, qemu-devel, Eric Blake, Markus Armbruster
Am 20.01.2023 um 14:35 hat Hanna Czenczek geschrieben:
> On 19.01.23 15:00, Kevin Wolf wrote:
> > Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> > > When a block driver supports obtaining format-specific information, but
> > > that object only contains optional fields, it is possible that none of
> > > them are present, so that dump_qobject() (called by
> > > bdrv_image_info_specific_dump()) will not print anything.
> > >
> > > The callers of bdrv_image_info_specific_dump() put a header above this
> > > information ("Format specific information:\n"), which will look strange
> > > when there is nothing below. Modify bdrv_image_info_specific_dump() to
> > > print this header instead of its callers, and only if there is indeed
> > > something to be printed.
> > >
> > > Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> > > index 2f0d8ac25a..084ec44d3b 100644
> > > --- a/qemu-io-cmds.c
> > > +++ b/qemu-io-cmds.c
> > > @@ -1819,8 +1819,8 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
> > > return -EIO;
> > > }
> > > if (spec_info) {
> > > - printf("Format specific information:\n");
> > > - bdrv_image_info_specific_dump(spec_info);
> > > + bdrv_image_info_specific_dump(spec_info,
> > > + "Format specific information:\n");
> > > qapi_free_ImageInfoSpecific(spec_info);
> > > }
> > Interesting observation here: That qemu-io uses printf() instead of
> > qemu_printf() for the top level, but then dump_qobject() (which results
> > in qemu_printf()) for the format specific information, means that if you
> > use the 'qemu-io' HMP command, you'll get half of the output on stdout
> > and the other half in the monitor.
>
> Hu. I can’t find a single instance of qemu_printf() in qemu-io-cmds.c, but
> then I assume all printf()s there should really be qemu_printf()?
That would probably be the most consistent way.
I expect it would change the output of some qemu-iotests cases, but we
can explicitly print whatever was returned in QMP to keep the same
information in the test output.
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 02/12] block/file: Add file-specific image info
2022-06-20 16:26 ` [PATCH v2 02/12] block/file: Add file-specific image info Hanna Reitz
@ 2023-01-27 23:00 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2023-01-27 23:00 UTC (permalink / raw)
To: Hanna Reitz; +Cc: qemu-block, qemu-devel, Kevin Wolf, Markus Armbruster
On Mon, Jun 20, 2022 at 06:26:54PM +0200, Hanna Reitz wrote:
> Add some (optional) information that the file driver can provide for
> image files, namely the extent size hint.
>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
> qapi/block-core.json | 26 ++++++++++++++++++++++++--
> block/file-posix.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 54 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 03/12] block/vmdk: Change extent info type
2023-01-19 15:20 ` Kevin Wolf
2023-01-20 13:43 ` Hanna Czenczek
@ 2023-01-27 23:06 ` Eric Blake
2023-01-30 10:56 ` Kevin Wolf
1 sibling, 1 reply; 24+ messages in thread
From: Eric Blake @ 2023-01-27 23:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Hanna Reitz, qemu-block, qemu-devel, Markus Armbruster
On Thu, Jan 19, 2023 at 04:20:16PM +0100, Kevin Wolf wrote:
> Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> > VMDK's implementation of .bdrv_get_specific_info() returns information
> > about its extent files, ostensibly in the form of ImageInfo objects.
> > However, it does not get this information through
> > bdrv_query_image_info(), but fills only a select few fields with custom
> > information that does not always match the fields' purposes.
> >
> > For example, @format, which is supposed to be a block driver name, is
> > filled with the extent type, e.g. SPARSE or FLAT.
> >
> > In ImageInfo, @compressed shows whether the data that can be seen in the
> > image is stored in compressed form or not. For example, a compressed
> > qcow2 image will store compressed data in its data file, but when
> > accessing the qcow2 node, you will see normal data. This is not how
> > VMDK uses the @compressed field for its extent files: Instead, it
> > signifies whether accessing the extent file will yield compressed data
> > (which the VMDK driver then (de-)compresses).
>
> Actually, VMDK was the only user of the field in ImageInfo. qcow2
> doesn't set the field at all because it would have to parse all L2
> tables in order to set it.
>
> So I suppose @compressed can now be removed from ImageInfo?
I think you are okay for VMDK (the new struct has the same field
names, but better meanings, and the on-the-wire representation for
someone querying a known-VMDK image doesn't change). For non-VMDK,
any code that was querying @compressed will break, but arguably no one
was doing that since it would have always been false. If we want to
be super-conservative, we deprecate the field now and only remove it
from ImageInfo in a later release, but I'd rather trust Markus on this
decision.
On a side note - would it be worth adding a bit to the qcow2 header
(one of the compatible_feature bits seems best) which we set when
writing a compressed cluster, so that on newer images, it is an O(1)
probe of whether the image contains (or at least has contained in the
past) a compressed cluster? Or is that going to add needless overhead
for something we really don't need to know all that often?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 03/12] block/vmdk: Change extent info type
2023-01-27 23:06 ` Eric Blake
@ 2023-01-30 10:56 ` Kevin Wolf
0 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2023-01-30 10:56 UTC (permalink / raw)
To: Eric Blake; +Cc: Hanna Reitz, qemu-block, qemu-devel, Markus Armbruster
Am 28.01.2023 um 00:06 hat Eric Blake geschrieben:
> On Thu, Jan 19, 2023 at 04:20:16PM +0100, Kevin Wolf wrote:
> > Am 20.06.2022 um 18:26 hat Hanna Reitz geschrieben:
> > > VMDK's implementation of .bdrv_get_specific_info() returns information
> > > about its extent files, ostensibly in the form of ImageInfo objects.
> > > However, it does not get this information through
> > > bdrv_query_image_info(), but fills only a select few fields with custom
> > > information that does not always match the fields' purposes.
> > >
> > > For example, @format, which is supposed to be a block driver name, is
> > > filled with the extent type, e.g. SPARSE or FLAT.
> > >
> > > In ImageInfo, @compressed shows whether the data that can be seen in the
> > > image is stored in compressed form or not. For example, a compressed
> > > qcow2 image will store compressed data in its data file, but when
> > > accessing the qcow2 node, you will see normal data. This is not how
> > > VMDK uses the @compressed field for its extent files: Instead, it
> > > signifies whether accessing the extent file will yield compressed data
> > > (which the VMDK driver then (de-)compresses).
> >
> > Actually, VMDK was the only user of the field in ImageInfo. qcow2
> > doesn't set the field at all because it would have to parse all L2
> > tables in order to set it.
> >
> > So I suppose @compressed can now be removed from ImageInfo?
>
> I think you are okay for VMDK (the new struct has the same field
> names, but better meanings, and the on-the-wire representation for
> someone querying a known-VMDK image doesn't change). For non-VMDK,
> any code that was querying @compressed will break, but arguably no one
> was doing that since it would have always been false. If we want to
> be super-conservative, we deprecate the field now and only remove it
> from ImageInfo in a later release, but I'd rather trust Markus on this
> decision.
It is an optional field and VMDK is the only driver that ever provided
it, and only in the context of extent information. In this case, the
information on the wire stays the same after this patch.
So I don't think there is any visible difference for a client, apart
from schema introspection?
> On a side note - would it be worth adding a bit to the qcow2 header
> (one of the compatible_feature bits seems best) which we set when
> writing a compressed cluster, so that on newer images, it is an O(1)
> probe of whether the image contains (or at least has contained in the
> past) a compressed cluster? Or is that going to add needless overhead
> for something we really don't need to know all that often?
I think the only use for it would be displaying it in 'qemu-img info'.
Would you combine two bits then? One for "compressed bit is valid" and
one for "contains compressed clusters"? Because with one bit you can
only distinguish "compressed" from "don't know", but there wouldn't be a
way to say for certain that an image is uncompressed.
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-01-30 10:57 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-20 16:26 [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 01/12] block: Improve empty format-specific info dump Hanna Reitz
2023-01-19 14:00 ` Kevin Wolf
2023-01-20 13:35 ` Hanna Czenczek
2023-01-20 14:08 ` Kevin Wolf
2022-06-20 16:26 ` [PATCH v2 02/12] block/file: Add file-specific image info Hanna Reitz
2023-01-27 23:00 ` Eric Blake
2022-06-20 16:26 ` [PATCH v2 03/12] block/vmdk: Change extent info type Hanna Reitz
2023-01-19 15:20 ` Kevin Wolf
2023-01-20 13:43 ` Hanna Czenczek
2023-01-27 23:06 ` Eric Blake
2023-01-30 10:56 ` Kevin Wolf
2022-06-20 16:26 ` [PATCH v2 04/12] block: Split BlockNodeInfo off of ImageInfo Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 05/12] qemu-img: Use BlockNodeInfo Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 06/12] block/qapi: Let bdrv_query_image_info() recurse Hanna Reitz
2022-06-20 16:26 ` [PATCH v2 07/12] block/qapi: Introduce BlockGraphInfo Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 08/12] block/qapi: Add indentation to bdrv_node_info_dump() Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 09/12] iotests: Filter child node information Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 10/12] iotests/106, 214, 308: Read only one size line Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 11/12] qemu-img: Let info print block graph Hanna Reitz
2022-06-20 16:27 ` [PATCH v2 12/12] qemu-img: Change info key names for protocol nodes Hanna Reitz
2022-12-08 12:24 ` [PATCH v2 00/12] qemu-img info: Show protocol-level information Hanna Reitz
2023-01-19 20:12 ` Kevin Wolf
2023-01-20 13:44 ` Hanna Czenczek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.