[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v5] qemu-img info lists bitmap directory entries
From: |
Andrey Shinkevich |
Subject: |
Re: [Qemu-block] [PATCH v5] qemu-img info lists bitmap directory entries |
Date: |
Tue, 11 Dec 2018 12:57:06 +0000 |
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