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

[Vladimir Sementsov-Ogievskiy]

07.12.2018 20:52, Vladimir Sementsov-Ogievskiy wrote:
> 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

and to make it simpler to recognize fail, it should be one thing, independent 
of the version, for example: omitted always means error. Or visa-versa: empty 
list always means error, omitted means no bitmaps or unsupported.

> 
>>
>>>
>>> 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