Re: [Qemu-devel] [PATCH v5] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

On 10.12.2018 22:48, Eric Blake wrote:
> On 12/10/18 12:50 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 10.12.2018 21:09, Andrey Shinkevich wrote:
>>> In the 'Format specific information' section of the 'qemu-img info'
>>> command output, the supplemental information about existing QCOW2
>>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>>
>>
>> [...]
>>
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -4270,6 +4270,19 @@ static ImageInfoSpecific 
>>> *qcow2_get_specific_info(BlockDriverState *bs)
>>>                .refcount_bits      = s->refcount_bits,
>>>            };
>>>        } else if (s->qcow_version == 3) {
>>> +        bool has_bitmaps;
>>> +        Qcow2BitmapInfoList *bitmaps;
>>> +        Error *local_err = NULL;
>>> +
>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>> +        if (local_err) {
>>> +            /* TODO: Report the Error up to the caller when 
>>> implemented */
>>> +            error_free(local_err);
>>> +            /* The 'bitmaps' empty list designates a failure to get 
>>> info */
>>> +            has_bitmaps = true;
> 
> I think you got it backwards.  I would prefer has_bitmaps = false on 
> error,...
> 
>>> +        } else {
>>> +            has_bitmaps = !!bitmaps;
> 
> ...and an empty list when you successfully determined that there are no 
> bitmaps.
> 
> 
>>> +++ b/qapi/block-core.json
>>> @@ -69,6 +69,11 @@
>>>    # @encrypt: details about encryption parameters; only set if image
>>>    #           is encrypted (since 2.10)
>>>    #
>>> +# @bitmaps: list of qcow2 bitmaps details; the empty list designates
>>> +#           "fail to load bitmaps" if it is passed to the caller or
>>> +#           "no bitmaps" otherwise;
>>> +#           unsupported for the format QCOW2 v2 (since 4.0)
>>
>>
>> For me, it looks simpler to declare alternative approach, assuming 
>> that absence
>> of the field means error, like this:
>>
>> @bitmaps: optional field with uncommon semantics:
>>             Absence of the field means that bitmaps info query failed 
>> (which doesn't
>>             imply the whole query failure).
>>             If the field exists in query results, there were no 
>> errors, and it represents
>>             list of qcow2 bitmaps details. So, successful result will 
>> always use empty
>>             list to show that there are no bitmaps.
>>             Note: bitmaps are not supported before QCOW2 v3, so for 
>> elder versions
>>             @bitmaps will always be an empty list.
> 
> I'd prefer:
> 
> @bitmaps: A list of qcow2 bitmap details (possibly empty, such as for v2
>            images which do not support bitmaps).  Absent if bitmap
>            information could not be obtained. (since 4.0)
> 

Thank you all for your comments. The interpretation of an entity depends 
on an eye of the beholder. I am flexible on the discussed matter and 
will include your approach into v6 version. Thank you very much for your 
collaboration.
-- 
With the best regards,
Andrey Shinkevich