[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: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v4] qemu-img info lists bitmap directory entries |
Date: |
Fri, 7 Dec 2018 11:31:42 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 |
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