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

[Eric Blake <eblake@redhat.com>]

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

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org