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