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

[Vladimir Sementsov-Ogievskiy]

07.12.2018 20:31, Eric Blake wrote:
> On 12/7/18 11:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
> 
> And thus we should probably fix that - but it doesn't have to be part of this 
> series.
> 
>>
>> 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.
>>
> 
>>>> -      '*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.
> 
> I'm happy with v2 images reporting a 0-length array instead of omitting the 
> field, especially if that means we can simply have:
> 
> old qemu: field always omitted because we didn't compute it
> new qemu: field omitted if we hit an error computing it
>            otherwise present (but possibly 0 length) and accurate

anyway, we can adjust human output type of qemu-img info to catch this case and 
print additional error message

> 
>>
>> 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.
> 
> No, I don't see a reason to over-engineer things; query-block is already a 
> behemoth without trying to jam in more details about whether info-exporting 
> hit partial failure.
> 


-- 
Best regards,
Vladimir