diff mbox series

[PULL,3/4] qcow2: Add list of bitmaps to ImageInfoSpecificQCow2

Message ID 20190211205128.27146-4-eblake@redhat.com
State New
Headers show
Series [PULL,1/4] nbd/server: Kill pointless shadowed variable | expand

Commit Message

Eric Blake Feb. 11, 2019, 8:51 p.m. UTC
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

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:

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
            granularity: 65536
        [1]:
            flags:
                [0]: in-use
                [1]: auto
            name: back-up2
            granularity: 65536
    refcount bits: 16
    corrupt: false

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <1549638368-530182-3-git-send-email-andrey.shinkevich@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json | 41 +++++++++++++++++++++++-
 block/qcow2.h        |  2 ++
 block/qcow2-bitmap.c | 76 ++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        | 11 ++++++-
 4 files changed, 128 insertions(+), 2 deletions(-)

Comments

Peter Maydell March 20, 2020, 5:57 p.m. UTC | #1
On Mon, 11 Feb 2019 at 20:57, Eric Blake <eblake@redhat.com> wrote:
>
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>
> 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:

Hi; Coverity has just noticed an issue (CID 1421894) with this change:

> diff --git a/block/qcow2.c b/block/qcow2.c
> index bcb80d0270c..65a54c9ac65 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4387,7 +4387,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
>      spec_info = g_new(ImageInfoSpecific, 1);
>      *spec_info = (ImageInfoSpecific){
>          .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
> -        .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
> +        .u.qcow2.data = g_new0(ImageInfoSpecificQCow2, 1),
>      };
>      if (s->qcow_version == 2) {
>          *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> @@ -4395,6 +4395,13 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
>              .refcount_bits      = s->refcount_bits,
>          };
>      } else if (s->qcow_version == 3) {
> +        Qcow2BitmapInfoList *bitmaps;
> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            qapi_free_ImageInfoSpecific(spec_info);
> +            return NULL;

If we take this error-exit codepath, then we never free the
memory allocated by the earlier call to qcrypto_block_get_info().

> +        }
>          *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>              .compat             = g_strdup("1.1"),
>              .lazy_refcounts     = s->compatible_features &
> @@ -4404,6 +4411,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
> --
> 2.20.1

thanks
-- PMM
Eric Blake March 20, 2020, 6:38 p.m. UTC | #2
On 3/20/20 12:57 PM, Peter Maydell wrote:
> On Mon, 11 Feb 2019 at 20:57, Eric Blake <eblake@redhat.com> wrote:
>>
>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>
>> 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:
> 
> Hi; Coverity has just noticed an issue (CID 1421894) with this change:
> 

>> +        Qcow2BitmapInfoList *bitmaps;
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            qapi_free_ImageInfoSpecific(spec_info);
>> +            return NULL;
> 
> If we take this error-exit codepath, then we never free the
> memory allocated by the earlier call to qcrypto_block_get_info().

Fix sent.

Hmm - it would be nice if the QAPI generator could declare all QAPI 
types as g_autoptr compatible, so we could simplify our cleanup paths to 
not have to worry about calling qapi_free_FOO() on all paths.  But while 
the memory leak fix is a one-liner safe for 5.0, switching to g_autoptr 
is a bigger task that would be 5.1 material.
diff mbox series

Patch

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5f17d67d714..0f349d46033 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -69,6 +69,8 @@ 
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @bitmaps: A list of qcow2 bitmap details (since 4.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +79,8 @@ 
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['Qcow2BitmapInfo']
   } }

 ##
@@ -453,6 +456,42 @@ 
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
            'status': 'DirtyBitmapStatus'} }

+##
+# @Qcow2BitmapInfoFlags:
+#
+# An enumeration of flags that a bitmap can report to the user.
+#
+# @in-use: This flag is set by any process actively modifying the qcow2 file,
+#          and cleared when the updated bitmap is flushed to the qcow2 image.
+#          The presence of this flag in an offline image means that the bitmap
+#          was not saved correctly after its last usage, and may contain
+#          inconsistent data.
+#
+# @auto: The bitmap must reflect all changes of the virtual disk by any
+#        application that would write to this qcow2 file.
+#
+# Since: 4.0
+##
+{ 'enum': 'Qcow2BitmapInfoFlags',
+  'data': ['in-use', 'auto'] }
+
+##
+# @Qcow2BitmapInfo:
+#
+# Qcow2 bitmap information.
+#
+# @name: the name of the bitmap
+#
+# @granularity: granularity of the bitmap in bytes
+#
+# @flags: flags of the bitmap
+#
+# Since: 4.0
+##
+{ 'struct': 'Qcow2BitmapInfo',
+  'data': {'name': 'str', 'granularity': 'uint32',
+           'flags': ['Qcow2BitmapInfoFlags'] } }
+
 ##
 # @BlockLatencyHistogramInfo:
 #
diff --git a/block/qcow2.h b/block/qcow2.h
index 32cce9eee22..9dd02df831c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -684,6 +684,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/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b9463014292..3ee524da4b5 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1006,6 +1006,82 @@  fail:
     return false;
 }

+
+static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
+{
+    Qcow2BitmapInfoFlagsList *list = NULL;
+    Qcow2BitmapInfoFlagsList **plist = &list;
+    int i;
+
+    static const struct {
+        int bme;  /* Bitmap directory entry flags */
+        int info; /* The flags to report to the user */
+    } map[] = {
+        { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
+        { BME_FLAG_AUTO,   QCOW2_BITMAP_INFO_FLAGS_AUTO },
+    };
+
+    int map_size = ARRAY_SIZE(map);
+
+    for (i = 0; i < map_size; ++i) {
+        if (flags & map[i].bme) {
+            Qcow2BitmapInfoFlagsList *entry =
+                g_new0(Qcow2BitmapInfoFlagsList, 1);
+            entry->value = map[i].info;
+            *plist = entry;
+            plist = &entry->next;
+            flags &= ~map[i].bme;
+        }
+    }
+    /* Check if the BME_* mapping above is complete */
+    assert(!flags);
+
+    return list;
+}
+
+/*
+ * qcow2_get_bitmap_info_list()
+ * Returns a list of QCOW2 bitmap details.
+ * In case of no bitmaps, the function returns NULL and
+ * the @errp parameter is not set.
+ * When bitmap information can not be obtained, the function returns
+ * NULL and the @errp parameter is set.
+ */
+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 & ~BME_RESERVED_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 bcb80d0270c..65a54c9ac65 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4387,7 +4387,7 @@  static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
     spec_info = g_new(ImageInfoSpecific, 1);
     *spec_info = (ImageInfoSpecific){
         .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
-        .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
+        .u.qcow2.data = g_new0(ImageInfoSpecificQCow2, 1),
     };
     if (s->qcow_version == 2) {
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
@@ -4395,6 +4395,13 @@  static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
             .refcount_bits      = s->refcount_bits,
         };
     } else if (s->qcow_version == 3) {
+        Qcow2BitmapInfoList *bitmaps;
+        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            qapi_free_ImageInfoSpecific(spec_info);
+            return NULL;
+        }
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("1.1"),
             .lazy_refcounts     = s->compatible_features &
@@ -4404,6 +4411,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