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