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

[Vladimir Sementsov-Ogievskiy]

06.12.2018 16:19, Andrey Shinkevich wrote:
> The 'Format specific information' of qemu-img info command will show
> the name, flags and granularity for every QCOW2 bitmap as follows:
> 
> image: /vz/vmprivate/VM1/harddisk.hdd
> file format: qcow2
> virtual size: 64G (68719476736 bytes)
> disk size: 3.0M
> cluster_size: 1048576
> Format specific information:
>      compat: 1.1
>      lazy refcounts: true
>      bitmaps:
>          [0]:
>              flags:
>                  [0]: in-use
>                  [1]: auto
>              name: back-up1
>              unknown flags: 4
>              granularity: 65536
>          [1]:
>              flags:
>                  [0]: in-use
>                  [1]: auto
>              name: back-up2
>              unknown flags: 8
>              granularity: 65536
>      refcount bits: 16
>      corrupt: false
> 
> Signed-off-by: Andrey Shinkevich <address@hidden>
> ---
> v3:
> Now, qcow2_get_bitmap_info_list() is invoked under the condition of QCOW
> version #3 to avoid memory leaks in case of QCOW version #2.
> Furthermore, qcow2_get_bitmap_info_list() checks the number of existing 
> bitmaps.
> So, if no bitmap exists, no bitmap error message is printed in the output.
> The data type of the bitmap 'granularity' parameter was left as 'uint32'
> because bitmap_list_load() returns error if granularity_bits is grater than 
> 31.
> 

Ok, uint32 works for me too.


[...]

> +
> +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> +                                                Error **errp)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    Qcow2BitmapList *bm_list;
> +    Qcow2Bitmap *bm;
> +    Qcow2BitmapInfoList *list = NULL;
> +    Qcow2BitmapInfoList **plist = &list;
> +
> +    if (s->nb_bitmaps == 0) {
> +        return NULL;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                               s->bitmap_directory_size, errp);
> +    if (bm_list == NULL) {
> +        return NULL;
> +    }
> +
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
> +        Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
> +        info->granularity = 1U << bm->granularity_bits;
> +        info->name = g_strdup(bm->name);
> +        info->flags = get_bitmap_info_flags(bm->flags);
> +        info->unknown_flags = bm->flags & ~(BME_FLAG_IN_USE | BME_FLAG_AUTO);

we should use bm->flags & BME_RESERVED_FLAGS instead

> +        info->has_unknown_flags = !!info->unknown_flags;
> +        obj->value = info;
> +        *plist = obj;
> +        plist = &obj->next;
> +    }
> +
> +    bitmap_list_free(bm_list);
> +
> +    return list;
> +}
> +
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>                                    Error **errp)
>   {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 991d6ac..a023856 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4254,6 +4254,8 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>       BDRVQcow2State *s = bs->opaque;
>       ImageInfoSpecific *spec_info;
>       QCryptoBlockInfo *encrypt_info = NULL;
> +    Error *local_err = NULL;
> +    Qcow2BitmapInfoList *bitmaps;
>   
>       if (s->crypto != NULL) {
>           encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
> @@ -4270,6 +4272,10 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>               .refcount_bits      = s->refcount_bits,
>           };
>       } else if (s->qcow_version == 3) {

both local_err and bitmaps variables are used only here.
I'd like at least bitmaps to be defined here too.

> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> +        if (local_err != NULL) {
> +            error_report_err(local_err);
> +        }
>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>               .compat             = g_strdup("1.1"),
>               .lazy_refcounts     = s->compatible_features &
> @@ -4279,6 +4285,8 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>                                     QCOW2_INCOMPAT_CORRUPT,
>               .has_corrupt        = true,
>               .refcount_bits      = s->refcount_bits,
> +            .has_bitmaps        = !!bitmaps,
> +            .bitmaps            = bitmaps,
>           };
>       } else {
>           /* if this assertion fails, this probably means a new version was
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 8662b68..0ec2b3d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -685,6 +685,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>                                     void **refcount_table,
>                                     int64_t *refcount_table_size);
>   bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> +                                                Error **errp);
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>                                    Error **errp);
>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d4fe710..55d7056 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

[...]

>   ##
> +# @Qcow2BitmapInfoFlags:
> +#
> +# An enumeration of states that a bitmap can report to the user.

may be, s/states/flags/ is closer to what it is.

> +#
> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
> +#
> +# @auto: The bitmap must reflect all changes of the virtual disk by any
> +#        application that would write to this qcow2 file.
> +#
> +# Since: 4.0
> +##


with at least BME_RESERVED_FLAGS used (and, if you want, other small things):

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir