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

[Vladimir Sementsov-Ogievskiy]

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;
> +        } else {
> +            has_bitmaps = !!bitmaps;
> +        }
>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>               .compat             = g_strdup("1.1"),
>               .lazy_refcounts     = s->compatible_features &
> @@ -4279,6 +4292,8 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>                                     QCOW2_INCOMPAT_CORRUPT,
>               .has_corrupt        = true,
>               .refcount_bits      = s->refcount_bits,
> +            .has_bitmaps        = has_bitmaps,
> +            .bitmaps            = bitmaps,
>           };
>       } else {
>           /* if this assertion fails, this probably means a new version was

[..]

> index d4fe710..53820a5 100644
> --- a/qapi/block-core.json
> +++ 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.


The main question here: is it a first time, we are doing something like this? 
If not,
we must go the existing way.

I've found the only one similar thing:
in qapi/misc.json:
# If @unavailable-features is an empty list, the CPU model is
# runnable using the current host and machine-type.
# If @unavailable-features is not present, runnability
# information for the CPU is not available.

it's not about error, however..

Interesting what is the common (most common) behavior around 
empty-list/absent-parameter?


Aha, one point to my semantics:
we can define required field, without '*', and it implies that there should not 
be any errors,
and we don't have extra options (only empty list is possible to show absence of 
elements).
and than, '*' shows possibility of errors (if described in spec).

and with your semantics, if we want to say in general, that empty-list = error, 
we'll need to
use '*' for all fields, even for thous without possible errors.

(of course, we actually can not say something in general, because, I'm afraid, 
that we currently
have mixed semantics around empty lists)


> +#
>   # Since: 1.7
>   ##
>   { 'struct': 'ImageInfoSpecificQCow2',
> @@ -77,7 +82,8 @@
>         '*lazy-refcounts': 'bool',
>         '*corrupt': 'bool',
>         'refcount-bits': 'int',
> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> +      '*bitmaps': ['Qcow2BitmapInfo']
>     } }
>   
>   ##
> @@ -454,6 +460,41 @@
>              'status': 'DirtyBitmapStatus'} }
>   



-- 
Best regards,
Vladimir