[Qemu-devel] [PATCH v10 0/3] qemu-img info lists bitmap directory entries

[Qemu-devel] [PATCH v10 0/3] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

v10:
The 'struct Error' parameter was added to the function prototype
bdrv_query_image_info().
The code refactoring of the function get_bitmap_info_flags().
The comments to the structures ImageInfoSpecificQCow2 and
Qcow2BitmapInfo in the file qapi/block-core.json were corrected.
The changes in the *.out files of the tests 060, 065 082, 198
and 206 were discarded. The new test 239 was enriched by adding human
readable format output and by checking the output with bitmap extra
parameters, such as  non-persistent and disabled.

v9:
The new test 239 of the qemu-iotests set was amended to show the bitmaps
being added and to demonstrate the bitmap flag "in-use".
The version #9 was discussed with the message ID:
<1548705688-1027522-1-git-send-email-andrey.shinkevich@virtuozzo.com>

v8:
The output benchmark files for the qemu-iotests, namely 060, 065 082, 198
and 206, were modified to show the bitmap extension for the qemu specific
information. A new test file 239 was added to the test set that checks the
output for the fields of the bitmap section.
The backward compatibility of the output for images of the version 2
of qcow2 was added.

v7:
A description was added to the function qcow2_get_bitmap_info_list().
In the function qcow2_get_specific_info(), the comment was modified
so that we ignore any error in obtaining the list of bitmaps to
pass the rest of QCOW2 specific information to a caller.

v6:
'[PATCH v6] qemu-img info lists bitmap directory entries'.
The error handling logic for the bitmaps empty list was reversed.

v5:
'[PATCH v5] qemu-img info lists bitmap directory entries'.
The error handling logic for the bitmaps empty list was fixed and documented.

v4:
'[PATCH v4] qemu-img info lists bitmap directory entries'.
Unknown flags are checked with the mask BME_RESERVED_FLAGS.
The code minor refactoring was made.

v3:
'[PATCH v3] qemu-img info lists bitmap directory entries'.
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.

v2:
'[PATCH v2] qemu-img info lists bitmap directory entries'.
The targeted version of the release at 'Since' word of the comments to the new
structures changed to 4.0 in the file qapi/block-core.json.
A comment to the 'bitmaps' new member was supplied.
The 'unknown flags' parameter was introduced to indicate presence of QCOW2
bitmap unknown flags, if any.
The word 'dirty' was removed from the code and from the comments as we list all
the bitmaps.
The 'bitmaps' printed parameter was removed for the release versions earlier
than 3.x.
The example of the output was moved above the 'Signed-off-by' line.

The first version was '[PATCH] qemu-img info lists bitmap directory entries'.

Andrey Shinkevich (3):
  bdrv_query_image_info Error parameter added
  qemu-img info lists bitmap directory entries
  qemu-img info: bitmaps extension new test 239

 block.c                    |   5 +-
 block/crypto.c             |   4 +-
 block/qapi.c               |   7 ++-
 block/qcow2-bitmap.c       |  77 +++++++++++++++++++++++++++
 block/qcow2.c              |  20 ++++++-
 block/qcow2.h              |   2 +
 block/vmdk.c               |   3 +-
 include/block/block.h      |   3 +-
 include/block/block_int.h  |   3 +-
 qapi/block-core.json       |  40 +++++++++++++-
 qemu-io-cmds.c             |   7 ++-
 tests/qemu-iotests/239     |  74 ++++++++++++++++++++++++++
 tests/qemu-iotests/239.out | 130 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 14 files changed, 364 insertions(+), 12 deletions(-)
 create mode 100755 tests/qemu-iotests/239
 create mode 100644 tests/qemu-iotests/239.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v10 1/3] bdrv_query_image_info Error parameter added

[Andrey Shinkevich]

Inform a user in case qcow2_get_specific_info fails to obtain
QCOW2 image specific information. This patch is preliminary to
the print of bitmap information in the 'qemu-img info' output.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block.c                   |  5 +++--
 block/crypto.c            |  4 ++--
 block/qapi.c              |  7 ++++++-
 block/qcow2.c             | 10 ++++++++--
 block/vmdk.c              |  3 ++-
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qemu-io-cmds.c            |  7 ++++++-
 8 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 4f5ff2c..1eb35ef 100644
--- a/block.c
+++ b/block.c
@@ -4307,11 +4307,12 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return drv->bdrv_get_info(bs, bdi);
 }
 
-ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs)
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
+                                          Error **errp)
 {
     BlockDriver *drv = bs->drv;
     if (drv && drv->bdrv_get_specific_info) {
-        return drv->bdrv_get_specific_info(bs);
+        return drv->bdrv_get_specific_info(bs, errp);
     }
     return NULL;
 }
diff --git a/block/crypto.c b/block/crypto.c
index f0a5f6b..4ede833 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -594,13 +594,13 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
 }
 
 static ImageInfoSpecific *
-block_crypto_get_specific_info_luks(BlockDriverState *bs)
+block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
 {
     BlockCrypto *crypto = bs->opaque;
     ImageInfoSpecific *spec_info;
     QCryptoBlockInfo *info;
 
-    info = qcrypto_block_get_info(crypto->block, NULL);
+    info = qcrypto_block_get_info(crypto->block, errp);
     if (!info) {
         return NULL;
     }
diff --git a/block/qapi.c b/block/qapi.c
index c66f949..f53f100 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -282,7 +282,12 @@ void bdrv_query_image_info(BlockDriverState *bs,
         info->dirty_flag = bdi.is_dirty;
         info->has_dirty_flag = true;
     }
-    info->format_specific     = bdrv_get_specific_info(bs);
+    info->format_specific     = bdrv_get_specific_info(bs, &err);
+    if (err) {
+        error_propagate(errp, err);
+        qapi_free_ImageInfo(info);
+        goto out;
+    }
     info->has_format_specific = info->format_specific != NULL;
 
     backing_filename = bs->backing_file;
diff --git a/block/qcow2.c b/block/qcow2.c
index 4897aba..27e5a2c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4367,14 +4367,20 @@ static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
-static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
+static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
+                                                  Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     ImageInfoSpecific *spec_info;
     QCryptoBlockInfo *encrypt_info = NULL;
+    Error *local_err = NULL;
 
     if (s->crypto != NULL) {
-        encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
+        encrypt_info = qcrypto_block_get_info(s->crypto, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return NULL;
+        }
     }
 
     spec_info = g_new(ImageInfoSpecific, 1);
diff --git a/block/vmdk.c b/block/vmdk.c
index 2c9e86d..544c10d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2314,7 +2314,8 @@ static int coroutine_fn vmdk_co_check(BlockDriverState *bs,
     return ret;
 }
 
-static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
+static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs,
+                                                 Error **errp)
 {
     int i;
     BDRVVmdkState *s = bs->opaque;
diff --git a/include/block/block.h b/include/block/block.h
index f70a843..9899c24 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -477,7 +477,8 @@ const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
-ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
+                                          Error **errp);
 void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t offset, int64_t bytes,
                             int64_t *cluster_offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622..0075baf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -319,7 +319,8 @@ struct BlockDriver {
                                   const char *name,
                                   Error **errp);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
-    ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
+    ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs,
+                                                 Error **errp);
 
     int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
                                           QEMUIOVector *qiov,
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2c39124..2187036 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1661,6 +1661,7 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
     BlockDriverState *bs = blk_bs(blk);
     BlockDriverInfo bdi;
     ImageInfoSpecific *spec_info;
+    Error *local_err = NULL;
     char s1[64], s2[64];
     int ret;
 
@@ -1682,7 +1683,11 @@ static int info_f(BlockBackend *blk, int argc, char **argv)
     printf("cluster size: %s\n", s1);
     printf("vm state offset: %s\n", s2);
 
-    spec_info = bdrv_get_specific_info(bs);
+    spec_info = bdrv_get_specific_info(bs, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -EIO;
+    }
     if (spec_info) {
         printf("Format specific information:\n");
         bdrv_image_info_specific_dump(fprintf, stdout, spec_info);
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 2/3] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

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
            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 <andrey.shinkevich@virtuozzo.com>
---
 block/qcow2-bitmap.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        | 10 +++++++
 block/qcow2.h        |  2 ++
 qapi/block-core.json | 40 ++++++++++++++++++++++++++-
 4 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b946301..17651cb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1006,6 +1006,83 @@ 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 = sizeof(map) / sizeof(map[0]);
+
+    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;
+        }
+    }
+
+    *plist = NULL;
+
+    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 (for a 0-length list in the QMP).
+ * When bitmap information can not be obtained, the function returns
+ * NULL and the @errp parameter is set (for omitting the list in QMP).
+ */
+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_RESERVED_FLAGS;
+        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 27e5a2c..4824ca8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4394,6 +4394,14 @@ 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);
+            g_free(spec_info->u.qcow2.data);
+            g_free(spec_info);
+            return NULL;
+        }
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("1.1"),
             .lazy_refcounts     = s->compatible_features &
@@ -4403,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
diff --git a/block/qcow2.h b/block/qcow2.h
index 438a1de..13e8964 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/qapi/block-core.json b/qapi/block-core.json
index 91685be..271e0df 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']
   } }
 
 ##
@@ -454,6 +457,41 @@
            'status': 'DirtyBitmapStatus'} }
 
 ##
+# @Qcow2BitmapInfoFlags:
+#
+# An enumeration of flags that a bitmap can report to the user.
+#
+# @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
+##
+{ 'enum': 'Qcow2BitmapInfoFlags',
+  'data': ['in-use', 'auto'] }
+
+##
+# @Qcow2BitmapInfo:
+#
+# Qcow2 bitmap information.
+#
+# @name: the name of the bitmap
+#
+# @granularity: granularity of the bitmap in bytes
+#
+# @flags: recognized flags of the bitmap
+#
+# @unknown-flags: any remaining flags not recognized by the current qemu version
+#
+# Since: 4.0
+##
+{ 'struct': 'Qcow2BitmapInfo',
+  'data': {'name': 'str', 'granularity': 'uint32',
+           'flags': ['Qcow2BitmapInfoFlags'],
+           '*unknown-flags': 'uint32' } }
+
+##
 # @BlockLatencyHistogramInfo:
 #
 # Block latency histogram.
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 3/3] qemu-img info: bitmaps extension new test 239

[Andrey Shinkevich]

A new test file 239 added to the qemu-iotests set. It checks
the output format of 'qemu-img info' for bitmaps extension of
qcow2 specific information.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/239     |  74 ++++++++++++++++++++++++++
 tests/qemu-iotests/239.out | 130 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 205 insertions(+)
 create mode 100755 tests/qemu-iotests/239
 create mode 100644 tests/qemu-iotests/239.out

diff --git a/tests/qemu-iotests/239 b/tests/qemu-iotests/239
new file mode 100755
index 0000000..bee7943
--- /dev/null
+++ b/tests/qemu-iotests/239
@@ -0,0 +1,74 @@
+#!/usr/bin/env python
+#
+# Test for qcow2 bitmap printed information
+#
+# Copyright (c) 2018 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+import json
+from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
+                    file_path, log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+disk = file_path('disk')
+chunk = 256
+
+def print_bitmap():
+    log('qemu-img info dump:\n')
+    iotests.img_info_log(disk, extra_args=['--force-share'])
+    result = json.loads(qemu_img_pipe('info', '--force-share',
+                                      '--output=json', disk))
+    if 'bitmaps' in result['format-specific']['data']:
+        bitmaps = result['format-specific']['data']['bitmaps']
+        log('The same bitmaps in JSON format:')
+        log(bitmaps, indent=2)
+    else:
+        log('No bitmap in JSON format output')
+
+def add_bitmap(bitmap_number, persistent, disabled):
+    granularity = 2**(13 + bitmap_number)
+    bitmap_name = 'bitmap-' + str(bitmap_number-1)
+    vm = iotests.VM().add_drive(disk)
+    vm.launch()
+    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
+               granularity=granularity, persistent=persistent,
+               disabled=disabled)
+    vm.shutdown()
+
+def write_to_disk(offset, size):
+    write = 'write {} {}K'.format(offset, size)
+    qemu_io('-f', iotests.imgfmt, '-c', write, disk)
+    log('Write ' + str(size) + 'K to disk at offset ' + str(hex(offset)))
+
+
+qemu_img_create('-f', iotests.imgfmt, disk, '1M')
+
+for num in range(1, 4):
+    disabled = False
+    if num == 2:
+        disabled = True
+    add_bitmap(num, bool(num-1), disabled)
+    write_to_disk((num-1)*chunk, chunk)
+    print_bitmap()
+    log('')
+
+vm = iotests.VM().add_drive(disk)
+vm.launch()
+log('Checking \"in-use\" flag...')
+print_bitmap()
+vm.shutdown()
diff --git a/tests/qemu-iotests/239.out b/tests/qemu-iotests/239.out
new file mode 100644
index 0000000..0abcac7
--- /dev/null
+++ b/tests/qemu-iotests/239.out
@@ -0,0 +1,130 @@
+{"execute": "block-dirty-bitmap-add", "arguments": {"disabled": false, "granularity": 16384, "name": "bitmap-0", "node": "drive0", "persistent": false}}
+{"return": {}}
+Write 256K to disk at offset 0x0
+qemu-img info dump:
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.0M (1048576 bytes)
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+No bitmap in JSON format output
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"disabled": true, "granularity": 32768, "name": "bitmap-1", "node": "drive0", "persistent": true}}
+{"return": {}}
+Write 256K to disk at offset 0x100
+qemu-img info dump:
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.0M (1048576 bytes)
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    bitmaps:
+        [0]:
+            flags:
+            name: bitmap-1
+            granularity: 32768
+    refcount bits: 16
+    corrupt: false
+
+The same bitmaps in JSON format:
+[
+  {
+    "flags": [],
+    "granularity": 32768,
+    "name": "bitmap-1"
+  }
+]
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"disabled": false, "granularity": 65536, "name": "bitmap-2", "node": "drive0", "persistent": true}}
+{"return": {}}
+Write 256K to disk at offset 0x200
+qemu-img info dump:
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.0M (1048576 bytes)
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    bitmaps:
+        [0]:
+            flags:
+            name: bitmap-1
+            granularity: 32768
+        [1]:
+            flags:
+                [0]: auto
+            name: bitmap-2
+            granularity: 65536
+    refcount bits: 16
+    corrupt: false
+
+The same bitmaps in JSON format:
+[
+  {
+    "flags": [],
+    "granularity": 32768,
+    "name": "bitmap-1"
+  },
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 65536,
+    "name": "bitmap-2"
+  }
+]
+
+Checking "in-use" flag...
+qemu-img info dump:
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1.0M (1048576 bytes)
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    bitmaps:
+        [0]:
+            flags:
+                [0]: in-use
+            name: bitmap-1
+            granularity: 32768
+        [1]:
+            flags:
+                [0]: in-use
+                [1]: auto
+            name: bitmap-2
+            granularity: 65536
+    refcount bits: 16
+    corrupt: false
+
+The same bitmaps in JSON format:
+[
+  {
+    "flags": [
+      "in-use"
+    ],
+    "granularity": 32768,
+    "name": "bitmap-1"
+  },
+  {
+    "flags": [
+      "in-use",
+      "auto"
+    ],
+    "granularity": 65536,
+    "name": "bitmap-2"
+  }
+]
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0f1c3f9..3e310c7 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -235,3 +235,4 @@
 235 auto quick
 236 auto quick
 238 auto quick
+239 rw auto quick
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v10 1/3] bdrv_query_image_info Error parameter added

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On 1/30/19 11:51 AM, Andrey Shinkevich wrote:
> Inform a user in case qcow2_get_specific_info fails to obtain
> QCOW2 image specific information. This patch is preliminary to
> the print of bitmap information in the 'qemu-img info' output.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v10 2/3] qemu-img info lists bitmap directory entries

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 6034 bytes --]

On 1/30/19 11:51 AM, 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:
> 

> +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 = sizeof(map) / sizeof(map[0]);
> +
> +    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;

Here's an idea for having runtime verification that our mapping of BME_
flags to QAPI flags is complete. At the spots marked [1], add:

flags &= ~map[i].bme;

> +        }
> +    }
> +
> +    *plist = NULL;

Dead assignment. plist is originally pointing to list (which was
NULL-initialized at declaration), and is only ever changed to point to
the list's next entry->next (which were NULL-initialized thanks to g_new0).

[1]
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 (for a 0-length list in the QMP).
> + * When bitmap information can not be obtained, the function returns
> + * NULL and the @errp parameter is set (for omitting the list in QMP).

Comment is a bit stale, now that we aren't going to omit the list in
QMP, but instead fail the QMP command outright.

> + */
> +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);

[1]
get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS)

> +        info->unknown_flags = bm->flags & BME_RESERVED_FLAGS;
> +        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 27e5a2c..4824ca8 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4394,6 +4394,14 @@ 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);
> +            g_free(spec_info->u.qcow2.data);
> +            g_free(spec_info);

I think it is cleaner to use qapi_free_ImageInfoSpecific(spec_info),
which takes care of recursion without you having to analyze whether two
g_free() calls are sufficient.  Of course, for THAT to work, we need to
fix the line above that does:

.u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),

to instead use g_new0(), since recursive freeing of uninitialized data
is not a good idea (without your patch, g_new() was sufficient since all
paths through the code either fully initialize or assert).  So maybe
your patch is the shortest that works, after all.


> +##
> +# @Qcow2BitmapInfo:
> +#
> +# Qcow2 bitmap information.
> +#
> +# @name: the name of the bitmap
> +#
> +# @granularity: granularity of the bitmap in bytes
> +#
> +# @flags: recognized flags of the bitmap
> +#
> +# @unknown-flags: any remaining flags not recognized by the current qemu version
> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'Qcow2BitmapInfo',
> +  'data': {'name': 'str', 'granularity': 'uint32',
> +           'flags': ['Qcow2BitmapInfoFlags'],
> +           '*unknown-flags': 'uint32' } }

Not for this patch, but how hard would it be to add a field showing the
number of set bits in the bitmap?

Kevin's insistence that a failure to read bitmap headers should fail the
overall 'qemu-img info' (on the grounds that we're going to have
problems using the image anyways) is reasonable enough; thanks for
putting up with competing demands (such as my earlier ideas of how best
to ignore read failures while still reporting as much remaining useful
information as possible), even if it has taken us through v10 to get to
a consensus on the semantics we want to support.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v10 2/3] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

On 30/01/2019 21:24, Eric Blake wrote:
> On 1/30/19 11:51 AM, 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:
>>
> 
>> +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 = sizeof(map) / sizeof(map[0]);
>> +
>> +    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;
> 
> Here's an idea for having runtime verification that our mapping of BME_
> flags to QAPI flags is complete. At the spots marked [1], add:
> 
> flags &= ~map[i].bme;
> 
>> +        }
>> +    }
>> +
>> +    *plist = NULL;
> 
> Dead assignment. plist is originally pointing to list (which was
> NULL-initialized at declaration), and is only ever changed to point to
> the list's next entry->next (which were NULL-initialized thanks to g_new0).

Yes, it is redundant due to the trailing '0' at g_new.
Agreed absolutely ))

> 
> [1]
> 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 (for a 0-length list in the QMP).
>> + * When bitmap information can not be obtained, the function returns
>> + * NULL and the @errp parameter is set (for omitting the list in QMP).
> 
> Comment is a bit stale, now that we aren't going to omit the list in
> QMP, but instead fail the QMP command outright.

Ouch! Missed out to correct that comment ((

> 
>> + */
>> +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);
> 
> [1]
> get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS)

Thank you, we will discuss it...

> 
>> +        info->unknown_flags = bm->flags & BME_RESERVED_FLAGS;
>> +        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 27e5a2c..4824ca8 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4394,6 +4394,14 @@ 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);
>> +            g_free(spec_info->u.qcow2.data);
>> +            g_free(spec_info);
> 
> I think it is cleaner to use qapi_free_ImageInfoSpecific(spec_info),
> which takes care of recursion without you having to analyze whether two
> g_free() calls are sufficient.  Of course, for THAT to work, we need to
> fix the line above that does:
> 
> .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
> 
> to instead use g_new0(), since recursive freeing of uninitialized data
> is not a good idea (without your patch, g_new() was sufficient since all
> paths through the code either fully initialize or assert).  So maybe
> your patch is the shortest that works, after all.

Yea, the invocation to qapi_free_ImageInfoSpecific(spec_info) looks like
a better code style.

> 
> 
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: recognized flags of the bitmap
>> +#
>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'Qcow2BitmapInfo',
>> +  'data': {'name': 'str', 'granularity': 'uint32',
>> +           'flags': ['Qcow2BitmapInfoFlags'],
>> +           '*unknown-flags': 'uint32' } }
> 
> Not for this patch, but how hard would it be to add a field showing the
> number of set bits in the bitmap?

I believe that is not too hard and would be happy to implement that with
another series, please.

> 
> Kevin's insistence that a failure to read bitmap headers should fail the
> overall 'qemu-img info' (on the grounds that we're going to have
> problems using the image anyways) is reasonable enough; thanks for
> putting up with competing demands (such as my earlier ideas of how best
> to ignore read failures while still reporting as much remaining useful
> information as possible), even if it has taken us through v10 to get to
> a consensus on the semantics we want to support.
> 
Both approaches looks reasonable to me.
"For by wise counsel thou shalt make thy war:
and in multitude of counselors there is safety."

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v10 2/3] qemu-img info lists bitmap directory entries

[Vladimir Sementsov-Ogievskiy]

30.01.2019 22:26, Andrey Shinkevich wrote:
> 
> 
> On 30/01/2019 21:24, Eric Blake wrote:
>> On 1/30/19 11:51 AM, Andrey Shinkevich wrote:
>>> In the 'Format specific information' section of the 'qemu-img info'
>>> command output, the supplemental information about existing QCOW2

[...]

>>> +##
>>> +# @Qcow2BitmapInfo:
>>> +#
>>> +# Qcow2 bitmap information.
>>> +#
>>> +# @name: the name of the bitmap
>>> +#
>>> +# @granularity: granularity of the bitmap in bytes
>>> +#
>>> +# @flags: recognized flags of the bitmap
>>> +#
>>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
>>> +#
>>> +# Since: 4.0
>>> +##
>>> +{ 'struct': 'Qcow2BitmapInfo',
>>> +  'data': {'name': 'str', 'granularity': 'uint32',
>>> +           'flags': ['Qcow2BitmapInfoFlags'],
>>> +           '*unknown-flags': 'uint32' } }
>>
>> Not for this patch, but how hard would it be to add a field showing the
>> number of set bits in the bitmap?
> 
> I believe that is not too hard and would be happy to implement that with
> another series, please.
> 

In common case, we should load all bitmaps for it, which is bad idea for qmp query
when vm is running.

On the other hand, in some meaningful cases (when open read-only, with qemu-img info),
we can reuse information obtained during loading bitmaps on open(). However, for in-use
bitmaps we'll have to load them to count dirty bits. Which is not a problem for qemu-img,
but again, may be not good idea while vm is running (as it may take too much time).

Also, exporting dirty-count for in-use bitmaps while vm is running may be ambiguous, as
some of the in-use bitmaps (or all) will be bitmaps in-use by exactly this vm run, so
actual dirty-count is in BdrvDirtyBitmap info.

And, another idea: we have extra_data field in bitmap list in qcow2,
which may be safely ignored if extra_data_compatible is set for this bitmap. So, it may
be a good reason to implement first type of compatible extra data.. But I don't sure that
it's worth it.

-- 
Best regards,
Vladimir