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

[Vladimir Sementsov-Ogievskiy]

07.12.2018 19:20, Eric Blake wrote:
> On 12/7/18 4:00 AM, Andrey Shinkevich wrote:
>> +++ 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.

Oh, yes, you are right. Strange, but  bdrv_get_specific_info lacks errp. 
error_abort us used above for crypto info errors.

Querying bitmaps needs disk access so it really may fail, and it may be sad to 
fail get any information because of repeating some disk/qcow2 error, so it's 
better not to abort and even not to fail qmp command here.

> 
> 
>> +++ 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).

Hmm, I don't like overusing .has_bitmaps as a sign of error, at least it would 
be very weird to document such behavior, and a undocumented trick it is not 
really useful.
Hmm, if we want something like this I'd prefer .has_bitmaps to be false only in 
case of error, so for v2 we'll have empty array. It's simpler to document.

Or we need separate cant_load_bitmaps: bool, or bitmaps should be enum of ( 
['Qcow2BitmapInfo'] , {'error': 'str'} ), do we have something like this 
already in QAPI? This is the question about partial success of info-exporting 
commands.


-- 
Best regards,
Vladimir