[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4] qemu-img info lists bitmap directory entries
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v4] qemu-img info lists bitmap directory entries |
Date: |
Fri, 7 Dec 2018 17:52:06 +0000 |
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