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

[Eric Blake]

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