Re: [Qemu-block] [PATCH v4] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

On 07.12.2018 19:20, Eric Blake wrote:
> On 12/7/18 4:00 AM, 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:
>>
>> image: /vz/vmprivate/VM1/harddisk.hdd
>> file format: qcow2
>> virtual size: 64G (68719476736 bytes)
>> disk size: 3.0M
>> cluster_size: 1048576
>> Format specific information:
>>      compat: 1.1
>>      lazy refcounts: true
>>      bitmaps:
>>          [0]:
>>              flags:
>>                  [0]: in-use
>>                  [1]: auto
>>              name: back-up1
>>              unknown flags: 4
>
> I'm guessing you doctored an image in a hex-editor to get this 
> particular output?
>
>>              granularity: 65536
>>          [1]:
>>              flags:
>>                  [0]: in-use
>>                  [1]: auto
>>              name: back-up2
>>              unknown flags: 8
>>              granularity: 65536
>>      refcount bits: 16
>>      corrupt: false
>>
>> Signed-off-by: Andrey Shinkevich <address@hidden>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>> v4:
>> Unknown flags are checked with the mask BME_RESERVED_FLAGS.
>> The code minor refactoring was made.
>>
>
>>   block/qcow2-bitmap.c | 56 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        |  8 ++++++++
>>   block/qcow2.h        |  2 ++
>>   qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 105 insertions(+), 1 deletion(-)
>
> I'm assuming John will merge this as a bitmap-related patch; make sure 
> he is in cc if you send a v5 (adding now).
>
>> +++ b/block/qcow2.c
>> @@ -4270,6 +4270,12 @@ static ImageInfoSpecific 
>> *qcow2_get_specific_info(BlockDriverState *bs)
>>               .refcount_bits      = s->refcount_bits,
>>           };
>>       } else if (s->qcow_version == 3) {
>> +        Qcow2BitmapInfoList *bitmaps;
>> +        Error *local_err = NULL;
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> +        if (local_err != NULL) {
>> +            error_report_err(local_err);
>> +        }
>
> Ouch. Calling error_report_err() doesn't always work in QMP context; 
> better would be to plumb Error **errp back up to the caller, if 
> getting this specific information can fail and we want the overall 
> query-block to fail.  Or, we could decide that failure to get bitmap 
> info is non-fatal, and that it was just a best-effort attempt to get 
> more info, where we then ignore the failure, rather than trying to 
> report it incorrectly.
>
>
>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: list of qcow2 bitmaps details (since 4.0)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +79,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['Qcow2BitmapInfo']
>
> Hmm. You're omitting this field both if there are 0 bitmaps, and when 
> it was a version 2 image. Is it worth including this field as a 
> 0-length array when there are no bitmaps but when the image format is 
> new enough to support them, or are we happy with the idea of only 
> including the field when it has at least one bitmap? The difference is 
> whether the calling app can explicitly learn that there are no bitmaps 
> (0-length reply) vs. the ambiguity of omitting it from the reply 
> (missing might mean no bitmaps, or an error in trying to report the 
> bitmaps, or an older qemu that didn't know how to report bitmaps).
>
>
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: flags of the bitmap
>> +#
>> +# @unknown-flags: unspecified flags if detected
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'Qcow2BitmapInfo',
>> +  'data': {'name': 'str', 'granularity': 'uint32',
>> +           'flags': ['Qcow2BitmapInfoFlags'],
>> +           '*unknown-flags': 'uint32' } }
>
> Here, you said flags will always be present, even if it is a 0-length 
> array. Did you test the case where an on-disk bitmap has neither 
> 'in-use' nor 'auto' set (where get_bitmap_info_flags() returns NULL) 
> to ensure that it indeed results in a 0-length reply and not a crash?
In case of no flag, the output for a bitmap looks like this:

image: /vz/vmprivate/VM1/harddisk.hdd
file format: qcow2
virtual size: 64G (68719476736 bytes)
disk size: 3.0M
cluster_size: 1048576
Format specific information:
     compat: 1.1
     lazy refcounts: true
     bitmaps:
         [0]:
             flags:
             name: x
             granularity: 65536
     refcount bits: 16
     corrupt: false

The output format is generated automatically based on the json syntax.
No crash happened.
>
> Otherwise, it's looking fairly good.
>

Kindly,

Andrey Shinkevich