[Top][All Lists]

[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: Eric Blake
Subject: Re: [Qemu-devel] [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 

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]