Re: [PATCH 2/4] block: Add protocol-specific image info

[Kevin Wolf]

Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben:
> The ImageInfo object currently only contains (optional) format-specific
> image information.  However, perhaps the protocol node can provide some
> additional information, so add a new field presenting it.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  qapi/block-core.json |  6 +++++-
>  block/qapi.c         | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index beeb91952a..e7d6c2e0cc 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -236,6 +236,9 @@
>  # @format-specific: structure supplying additional format-specific
>  #                   information (since 1.7)
>  #
> +# @protocol-specific: structure supplying additional protocol-specific
> +#                     information (since 7.1)
> +#
>  # Since: 1.3
>  #
>  ##
> @@ -246,7 +249,8 @@
>             '*backing-filename': 'str', '*full-backing-filename': 'str',
>             '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
>             '*backing-image': 'ImageInfo',
> -           '*format-specific': 'ImageInfoSpecific' } }
> +           '*format-specific': 'ImageInfoSpecific',
> +           '*protocol-specific': 'ImageInfoSpecific' } }

I'm not a fan of this one. It solves the problem for exactly one special
case (even if admittedly a common one) and leaves everything else as it
is. It is unclear what it produces in configurations that aren't the
simple one format node on top of one protocol node layout.

I would rather interpret 'format-specific' as 'driver-specific' and make
the ImageInfo for any child node accessible.

With rbd we already interpret it like a generic driver thing that is not
just for formats that because it implements .bdrv_get_specific_info even
though we didn't have a 'protocol-specific' yet.

Making other nodes has precedence, too. 'backing-image' is even in the
context of this hunk. VMDK exposes its extents the same way. So maybe
what we really want is a 'children' list with the ImageInfo of every
child node. And then qemu-img could go through all children and print
headings like "Driver specific information for file (#block123)". Then
filters like blkdebug could add their information and it would be
printed, too.

>  ##
>  # @ImageCheck:
> diff --git a/block/qapi.c b/block/qapi.c
> index 51202b470a..293983cf82 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>      int64_t size;
>      const char *backing_filename;
>      BlockDriverInfo bdi;
> +    BlockDriverState *protocol_bs;
>      int ret;
>      Error *err = NULL;
>      ImageInfo *info;
> @@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs,
>      }
>      info->has_format_specific = info->format_specific != NULL;
>  
> +    /* Try to look for an unambiguous protocol node */
> +    protocol_bs = bs;
> +    while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) {
> +        protocol_bs = bdrv_primary_bs(protocol_bs);
> +    }

If bs is already a leaf node, this duplicates the information, which
looks weird:

    $ build/qemu-img info -f file ~/tmp/test.raw
    image: /home/kwolf/tmp/test.raw
    file format: file
    virtual size: 10 GiB (10737418240 bytes)
    disk size: 7.63 GiB
    Format specific information:
        extent size: 1048576
    Protocol specific information:
        extent size: 1048576

>
> +    if (protocol_bs) {
> +        /* Assert that this is a protocol node */
> +        assert(QLIST_EMPTY(&protocol_bs->children));
> +
> +        info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            qapi_free_ImageInfo(info);
> +            goto out;
> +        }
> +        info->has_protocol_specific = info->protocol_specific != NULL;
> +    }
> +
>      backing_filename = bs->backing_file;
>      if (backing_filename[0] != '\0') {
>          char *backing_filename2;

Kevin