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

["Denis V. Lunev" <den@virtuozzo.com>]

On 7/6/23 15:10, Andrey Drobyshev wrote:
> On 6/21/23 21:12, Denis V. Lunev wrote:
>> 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.
>>
> Yes, I agree that making this field optional makes sense since we do not
> include it for formats which don't support compression.
>
> However, I don't think we should entirely omit it for uncompressed
> clusters.  If we keep it present that would be more consistent with the
> current logic, as with '"zero": false' field which is always included.
you right, that makes sense

Den