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

[Hanna Czenczek <hreitz@redhat.com>]

On 29.08.23 08:44, Andrey Drobyshev wrote:
> On 8/25/23 17:14, Hanna Czenczek wrote:
>> On 06.07.23 18:30, 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(-)
>> Patch 3 must be merged into this patch.  Every test must pass on every
>> commit so we don’t break bisecting.
> Agreed, should've figured that myself.
>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 5dd5f7e4b0..b263d2cd30 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.  Optional,
>>> +#     only valid for the formats whith support compression
>> This is missing information for when this field was introduced (i.e. a
>> “(since 8.2)”).
> Noted.
>
>> I also wonder why this field is optional.  We have compression
>> information even for formats that don’t support compression,
>> specifically, nothing is compressed.  I would just make this field
>> mandatory and print it always.  (A technical reason to do so is that
>> this patch uses block_driver_can_compress() to figure out whether there
>> is compression support; but that function only tells whether the driver
>> can write compressed data.  Even if it cannot do that, the format may
>> still support compression, and the driver may be able to read compressed
>> data, just not write it.)
>>
> I figured that for the formats which surely can't support compression,
> such as "raw", this information would simply be redundant.  AFAICT right
> now the only drivers which can read compressed data are exactly the ones
> which can write it: vmdk, qcow and qcow2.  But if we assume that this
> might change, and that we'd better show the field value no matter what
> (as we do with "zero" field) -- I'm OK with it.

It is indeed redundant, but as this is intended to be the 
machine-readable output, I don’t mind the output becoming arguably 
needlessly verbose.