Re: [PATCH 2/3] qemu-img: map: report compressed data blocks

[Denis V. Lunev]

On 6/7/23 17:26, Andrey Drobyshev wrote:
> Right now "qemu-img map" reports compressed blocks as containing data
> but having no host offset.  This is not very informative.  Instead,
> let's add another boolean field named "compressed" in case JSON output
> mode is specified.  This is achieved by utilizing new allocation status
> flag BDRV_BLOCK_COMPRESSED for bdrv_block_status().
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   qapi/block-core.json |  7 +++++--
>   qemu-img.c           | 16 +++++++++++++---
>   2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5dd5f7e4b0..bc6653e5d6 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -409,6 +409,9 @@
>   #
>   # @zero: whether the virtual blocks read as zeroes
>   #
> +# @compressed: true indicates that data is stored compressed (the target
> +#     format must support compression)
> +#
>   # @depth: number of layers (0 = top image, 1 = top image's backing
>   #     file, ..., n - 1 = bottom image (where n is the number of images
>   #     in the chain)) before reaching one for which the range is
> @@ -426,8 +429,8 @@
>   ##
>   { 'struct': 'MapEntry',
>     'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> -           'zero': 'bool', 'depth': 'int', 'present': 'bool',
> -           '*offset': 'int', '*filename': 'str' } }
> +           'zero': 'bool', 'compressed': 'bool', 'depth': 'int',
> +           'present': 'bool', '*offset': 'int', '*filename': 'str' } }
after some thoughts I would say that for compatibility reasons it
would be beneficial to have compressed field optional.
>   
>   ##
>   # @BlockdevCacheInfo:
> diff --git a/qemu-img.c b/qemu-img.c
> index 27f48051b0..9bb69f58f6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3083,7 +3083,7 @@ static int img_info(int argc, char **argv)
>   }
>   
>   static int dump_map_entry(OutputFormat output_format, MapEntry *e,
> -                          MapEntry *next)
> +                          MapEntry *next, bool can_compress)
>   {
>       switch (output_format) {
>       case OFORMAT_HUMAN:
> @@ -3112,6 +3112,9 @@ static int dump_map_entry(OutputFormat output_format, 
> MapEntry *e,
>                  e->present ? "true" : "false",
>                  e->zero ? "true" : "false",
>                  e->data ? "true" : "false");
> +        if (can_compress) {
> +            printf(", \"compressed\": %s", e->compressed ? "true" : "false");
If compressed field is optional, then it would be reasonable to skip
filling this field for non-compressed clusters. In that case we
will not need 'can_compress' parameter of the call.

Ha! More importantly. The field (according to the metadata) is
mandatory while it is reported conditionally, i.e. the field is
optional in reality. There is a problem in a this or that way.

> +        }
>           if (e->has_offset) {
>               printf(", \"offset\": %"PRId64"", e->offset);
>           }
> @@ -3172,6 +3175,7 @@ static int get_block_status(BlockDriverState *bs, int64_t 
> offset,
>           .length = bytes,
>           .data = !!(ret & BDRV_BLOCK_DATA),
>           .zero = !!(ret & BDRV_BLOCK_ZERO),
> +        .compressed = !!(ret & BDRV_BLOCK_COMPRESSED),
>           .offset = map,
>           .has_offset = has_offset,
>           .depth = depth,
> @@ -3189,6 +3193,7 @@ static inline bool entry_mergeable(const MapEntry *curr, 
> const MapEntry *next)
>       }
>       if (curr->zero != next->zero ||
>           curr->data != next->data ||
> +        curr->compressed != next->compressed ||
>           curr->depth != next->depth ||
>           curr->present != next->present ||
>           !curr->filename != !next->filename ||
> @@ -3218,6 +3223,7 @@ static int img_map(int argc, char **argv)
>       bool force_share = false;
>       int64_t start_offset = 0;
>       int64_t max_length = -1;
> +    bool can_compress = false;
>   
>       fmt = NULL;
>       output = NULL;
> @@ -3313,6 +3319,10 @@ static int img_map(int argc, char **argv)
>           length = MIN(start_offset + max_length, length);
>       }
>   
> +    if (output_format == OFORMAT_JSON) {
> +        can_compress = block_driver_can_compress(bs->drv);
> +    }
> +
>       curr.start = start_offset;
>       while (curr.start + curr.length < length) {
>           int64_t offset = curr.start + curr.length;
> @@ -3330,7 +3340,7 @@ static int img_map(int argc, char **argv)
>           }
>   
>           if (curr.length > 0) {
> -            ret = dump_map_entry(output_format, &curr, &next);
> +            ret = dump_map_entry(output_format, &curr, &next, can_compress);
>               if (ret < 0) {
>                   goto out;
>               }
> @@ -3338,7 +3348,7 @@ static int img_map(int argc, char **argv)
>           curr = next;
>       }
>   
> -    ret = dump_map_entry(output_format, &curr, NULL);
> +    ret = dump_map_entry(output_format, &curr, NULL, can_compress);
>       if (output_format == OFORMAT_JSON) {
>           puts("]");
>       }