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

[Eric Blake]

On 12/10/18 12:50 PM, Vladimir Sementsov-Ogievskiy wrote:
> 10.12.2018 21:09, Andrey Shinkevich wrote:
> > In the 'Format specific information' section of the 'qemu-img info'
> > command output, the supplemental information about existing QCOW2
> > bitmaps will be shown, such as a bitmap name, flags and granularity:
>
> [...]
>
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -4270,6 +4270,19 @@ static ImageInfoSpecific 
> > *qcow2_get_specific_info(BlockDriverState *bs)
> >                .refcount_bits      = s->refcount_bits,
> >            };
> >        } else if (s->qcow_version == 3) {
> > +        bool has_bitmaps;
> > +        Qcow2BitmapInfoList *bitmaps;
> > +        Error *local_err = NULL;
> > +
> > +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> > +        if (local_err) {
> > +            /* TODO: Report the Error up to the caller when implemented */
> > +            error_free(local_err);
> > +            /* The 'bitmaps' empty list designates a failure to get info */
> > +            has_bitmaps = true;

I think you got it backwards.  I would prefer has_bitmaps = false on 
error,...

> > +        } else {
> > +            has_bitmaps = !!bitmaps;

...and an empty list when you successfully determined that there are no 
bitmaps.


> > +++ b/qapi/block-core.json
> > @@ -69,6 +69,11 @@
> >    # @encrypt: details about encryption parameters; only set if image
> >    #           is encrypted (since 2.10)
> >    #
> > +# @bitmaps: list of qcow2 bitmaps details; the empty list designates
> > +#           "fail to load bitmaps" if it is passed to the caller or
> > +#           "no bitmaps" otherwise;
> > +#           unsupported for the format QCOW2 v2 (since 4.0)
>
>
> For me, it looks simpler to declare alternative approach, assuming that absence
> of the field means error, like this:
>
> @bitmaps: optional field with uncommon semantics:
>             Absence of the field means that bitmaps info query failed (which 
> doesn't
>             imply the whole query failure).
>             If the field exists in query results, there were no errors, and it 
> represents
>             list of qcow2 bitmaps details. So, successful result will always 
> use empty
>             list to show that there are no bitmaps.
>             Note: bitmaps are not supported before QCOW2 v3, so for elder 
> versions
>             @bitmaps will always be an empty list.

I'd prefer:

@bitmaps: A list of qcow2 bitmap details (possibly empty, such as for v2
          images which do not support bitmaps).  Absent if bitmap
          information could not be obtained. (since 4.0)

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org