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\ufffd\ufffd\ufffds 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\ufffd\ufffd\ufffdt be there. It
wouldn\ufffd\ufffd\ufffdt 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\ufffd\ufffd\ufffdt 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\ufffd\ufffd\ufffds ImageInfo. It
would be strange to link the backing node\ufffd\ufffd\ufffds 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\ufffd\ufffd\ufffdt 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\ufffd\ufffd\ufffdt an incompatible change, but I\ufffd\ufffd\ufffdm 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\ufffd\ufffd\ufffdt
have to worry about the fact that the backing node\ufffd\ufffd\ufffds ImageInfo were
duplicated.
There is another potential duplication problem, though: VMDK\ufffd\ufffd\ufffds
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\ufffd\ufffd\ufffdt
actually the extent nodes\ufffd\ufffd\ufffd 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\ufffd\ufffd\ufffdt know whether that
is technically an incompatible change, and I don\ufffd\ufffd\ufffdt 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\ufffd\ufffd\ufffds
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 \ufffd\ufffd\ufffdextent size\ufffd\ufffd\ufffd corrected
to \ufffd\ufffd\ufffdextent size hint\ufffd\ufffd\ufffd):
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\ufffd\ufffd\ufffdt a good way to go about it (and I can\ufffd\ufffd\ufffdt
find any better reasonable way to only print that driver-specific
information (and nothing else) from the protocol node).
For completeness\ufffd\ufffd\ufffds 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