[Qemu-devel] [PATCH v9 0/2] qemu-img info lists bitmap directory entries

[Qemu-devel] [PATCH v9 0/2] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

Currently, I am tackling the implementation of the Eric Blake's ideas,
who proposed the following:
"Should the test also create a disabled bitmap ("enabled":false), to show
the change in flags, and/or a non-persistent bitmap (to show that it
does not affect the qcow2 information, but only the query-blocks output,
because it is transient)?" which were with the the message ID:
<c5c4a099-d89c-6991-6644-98fc760b9106@redhat.com>
The version #7 was discussed with the message ID:
<1544698788-52893-1-git-send-email-andrey.shinkevich@virtuozzo.com>

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".

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 (2):
  qemu-img info lists bitmap directory entries
  qemu-img info: bitmaps extension new test 239

 block/qapi.c               |  6 ++++
 block/qcow2-bitmap.c       | 64 +++++++++++++++++++++++++++++++++
 block/qcow2.c              | 13 +++++++
 block/qcow2.h              |  2 ++
 qapi/block-core.json       | 42 +++++++++++++++++++++-
 tests/qemu-iotests/060.out |  1 +
 tests/qemu-iotests/065     | 16 ++++-----
 tests/qemu-iotests/082.out |  7 ++++
 tests/qemu-iotests/198.out |  2 ++
 tests/qemu-iotests/206.out |  5 +++
 tests/qemu-iotests/239     | 60 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/239.out | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 13 files changed, 298 insertions(+), 9 deletions(-)
 create mode 100755 tests/qemu-iotests/239
 create mode 100644 tests/qemu-iotests/239.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v9 1/2] 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

As the print of the qcow2 specific information expanded by adding
the bitmap parameters to the 'qemu-img info' command output,
it requires amendment of the output benchmark in the following
tests: 060, 065, 082, 198, and 206.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/qapi.c               |  6 +++++
 block/qcow2-bitmap.c       | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c              | 13 ++++++++++
 block/qcow2.h              |  2 ++
 qapi/block-core.json       | 42 +++++++++++++++++++++++++++++-
 tests/qemu-iotests/060.out |  1 +
 tests/qemu-iotests/065     | 16 ++++++------
 tests/qemu-iotests/082.out |  7 +++++
 tests/qemu-iotests/198.out |  2 ++
 tests/qemu-iotests/206.out |  5 ++++
 10 files changed, 149 insertions(+), 9 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index c66f949..0fde98c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -38,6 +38,7 @@
 #include "qapi/qmp/qstring.h"
 #include "sysemu/block-backend.h"
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 
 BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
                                         BlockDriverState *bs, Error **errp)
@@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
 
     if (info->has_format_specific) {
         func_fprintf(f, "Format specific information:\n");
+        if (info->format_specific &&
+            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
+            info->format_specific->u.qcow2.data->has_bitmaps == false) {
+            warn_report("Failed to load bitmap list");
+        }
         bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
     }
 }
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b946301..ae842eb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1006,6 +1006,70 @@ fail:
     return false;
 }
 
+static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
+{
+    Qcow2BitmapInfoFlagsList *list = NULL;
+    Qcow2BitmapInfoFlagsList **plist = &list;
+
+    if (flags & BME_FLAG_IN_USE) {
+        Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+        entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
+        *plist = entry;
+        plist = &entry->next;
+    }
+    if (flags & BME_FLAG_AUTO) {
+        Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+        entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
+        *plist = entry;
+    }
+    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 4897aba..07b99ee 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("0.10"),
             .refcount_bits      = s->refcount_bits,
+            .has_bitmaps        = true, /* To handle error check properly */
+            .bitmaps            = NULL, /* Unsupported for version 2 */
         };
     } else if (s->qcow_version == 3) {
+        Qcow2BitmapInfoList *bitmaps;
+        Error *local_err = NULL;
+
+        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("1.1"),
             .lazy_refcounts     = s->compatible_features &
@@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
                                   QCOW2_INCOMPAT_CORRUPT,
             .has_corrupt        = true,
             .refcount_bits      = s->refcount_bits,
+            .has_bitmaps        = !local_err,
+            .bitmaps            = bitmaps,
         };
+        /*
+         * If an error occurs in obtaining bitmaps, ignore
+         * it to show other QCOW2 specific information.
+         */
+        error_free(local_err);
     } else {
         /* if this assertion fails, this probably means a new version was
          * added without having it covered here */
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..3352562 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -69,6 +69,10 @@
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @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)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +81,8 @@
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['Qcow2BitmapInfo']
   } }
 
 ##
@@ -454,6 +459,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: flags of the bitmap
+#
+# @unknown-flags: unspecified flags if detected
+#
+# Since: 4.0
+##
+{ 'struct': 'Qcow2BitmapInfo',
+  'data': {'name': 'str', 'granularity': 'uint32',
+           'flags': ['Qcow2BitmapInfoFlags'],
+           '*unknown-flags': 'uint32' } }
+
+##
 # @BlockLatencyHistogramInfo:
 #
 # Block latency histogram.
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index af623cf..d0438b2 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -18,6 +18,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: false
+    bitmaps:
     refcount bits: 16
     corrupt: true
 can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 8bac383..86406cb 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -88,23 +88,23 @@ class TestQMP(TestImageInfoSpecific):
 class TestQCow2(TestQemuImgInfo):
     '''Testing a qcow2 version 2 image'''
     img_options = 'compat=0.10'
-    json_compare = { 'compat': '0.10', 'refcount-bits': 16 }
-    human_compare = [ 'compat: 0.10', 'refcount bits: 16' ]
+    json_compare = { 'compat': '0.10', 'bitmaps': [], 'refcount-bits': 16 }
+    human_compare = [ 'compat: 0.10', 'bitmaps:', 'refcount bits: 16' ]
 
 class TestQCow3NotLazy(TestQemuImgInfo):
     '''Testing a qcow2 version 3 image with lazy refcounts disabled'''
     img_options = 'compat=1.1,lazy_refcounts=off'
-    json_compare = { 'compat': '1.1', 'lazy-refcounts': False,
+    json_compare = { 'compat': '1.1', 'lazy-refcounts': False, 'bitmaps': [],
                      'refcount-bits': 16, 'corrupt': False }
-    human_compare = [ 'compat: 1.1', 'lazy refcounts: false',
+    human_compare = [ 'compat: 1.1', 'lazy refcounts: false', 'bitmaps:',
                       'refcount bits: 16', 'corrupt: false' ]
 
 class TestQCow3Lazy(TestQemuImgInfo):
     '''Testing a qcow2 version 3 image with lazy refcounts enabled'''
     img_options = 'compat=1.1,lazy_refcounts=on'
-    json_compare = { 'compat': '1.1', 'lazy-refcounts': True,
+    json_compare = { 'compat': '1.1', 'lazy-refcounts': True, 'bitmaps': [],
                      'refcount-bits': 16, 'corrupt': False }
-    human_compare = [ 'compat: 1.1', 'lazy refcounts: true',
+    human_compare = [ 'compat: 1.1', 'lazy refcounts: true', 'bitmaps:',
                       'refcount bits: 16', 'corrupt: false' ]
 
 class TestQCow3NotLazyQMP(TestQMP):
@@ -112,7 +112,7 @@ class TestQCow3NotLazyQMP(TestQMP):
        with lazy refcounts enabled'''
     img_options = 'compat=1.1,lazy_refcounts=off'
     qemu_options = 'lazy-refcounts=on'
-    compare = { 'compat': '1.1', 'lazy-refcounts': False,
+    compare = { 'compat': '1.1', 'lazy-refcounts': False, 'bitmaps': [],
                 'refcount-bits': 16, 'corrupt': False }
 
 
@@ -121,7 +121,7 @@ class TestQCow3LazyQMP(TestQMP):
        with lazy refcounts disabled'''
     img_options = 'compat=1.1,lazy_refcounts=on'
     qemu_options = 'lazy-refcounts=off'
-    compare = { 'compat': '1.1', 'lazy-refcounts': True,
+    compare = { 'compat': '1.1', 'lazy-refcounts': True, 'bitmaps': [],
                 'refcount-bits': 16, 'corrupt': False }
 
 TestImageInfoSpecific = None
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 0ce18c0..cfae9c4 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -18,6 +18,7 @@ cluster_size: 4096
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -30,6 +31,7 @@ cluster_size: 8192
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -276,6 +278,7 @@ cluster_size: 4096
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -287,6 +290,7 @@ cluster_size: 8192
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -529,6 +533,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -540,6 +545,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: false
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -551,6 +557,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out
index adb805c..aeee3c6 100644
--- a/tests/qemu-iotests/198.out
+++ b/tests/qemu-iotests/198.out
@@ -36,6 +36,7 @@ image: json:{"encrypt.key-secret": "sec0", "driver": "IMGFMT", "file": {"driver"
 file format: IMGFMT
 virtual size: 16M (16777216 bytes)
 Format specific information:
+    bitmaps:
     encrypt:
         ivgen alg: plain64
         hash alg: sha256
@@ -79,6 +80,7 @@ file format: IMGFMT
 virtual size: 16M (16777216 bytes)
 backing file: TEST_DIR/t.IMGFMT.base
 Format specific information:
+    bitmaps:
     encrypt:
         ivgen alg: plain64
         hash alg: sha256
diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out
index 91f4db5..9758569 100644
--- a/tests/qemu-iotests/206.out
+++ b/tests/qemu-iotests/206.out
@@ -19,6 +19,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: false
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -41,6 +42,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: false
+    bitmaps:
     refcount bits: 16
     corrupt: false
 
@@ -63,6 +65,7 @@ cluster_size: 2097152
 Format specific information:
     compat: 1.1
     lazy refcounts: true
+    bitmaps:
     refcount bits: 1
     corrupt: false
 
@@ -86,6 +89,7 @@ backing file: TEST_IMG.base
 backing file format: IMGFMT
 Format specific information:
     compat: 0.10
+    bitmaps:
     refcount bits: 16
 
 === Successful image creation (encrypted) ===
@@ -103,6 +107,7 @@ cluster_size: 65536
 Format specific information:
     compat: 1.1
     lazy refcounts: false
+    bitmaps:
     refcount bits: 16
     encrypt:
         ivgen alg: plain64
-- 
1.8.3.1

[Qemu-devel] [PATCH v9 2/2] 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     | 60 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/239.out | 88 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 149 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..233b58b
--- /dev/null
+++ b/tests/qemu-iotests/239
@@ -0,0 +1,60 @@
+#!/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
+
+disk = file_path('disk')
+chunk = 256
+
+def print_bitmap():
+    log('bitmap info dump:')
+    result = json.loads(qemu_img_pipe('info', '--force-share',
+                                      '--output=json', disk))
+    bitmaps = result['format-specific']['data']['bitmaps']
+    log(bitmaps, indent=2)
+
+def add_bitmap(bitmap_number):
+    num = bitmap_number
+    granularity = 2**(13 + num)
+    bitmap_name = 'bitmap-' + str(num)
+    vm = iotests.VM().add_drive(disk)
+    vm.launch()
+    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
+               granularity=granularity, persistent=True)
+    vm.shutdown()
+    write = 'write {} {}K'.format((num-1)*chunk, chunk)
+    qemu_io('-f', iotests.imgfmt, '-c', write, disk)
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+qemu_img_create('-f', iotests.imgfmt, disk, '1M')
+
+for num in range(1, 4):
+    add_bitmap(num)
+    print_bitmap()
+    log('')
+
+vm = iotests.VM().add_drive(disk)
+vm.launch()
+log('Check \"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..efeca9f
--- /dev/null
+++ b/tests/qemu-iotests/239.out
@@ -0,0 +1,88 @@
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 16384, "name": "bitmap-1", "node": "drive0", "persistent": true}}
+{"return": {}}
+bitmap info dump:
+[
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 16384,
+    "name": "bitmap-1"
+  }
+]
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 32768, "name": "bitmap-2", "node": "drive0", "persistent": true}}
+{"return": {}}
+bitmap info dump:
+[
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 16384,
+    "name": "bitmap-1"
+  },
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 32768,
+    "name": "bitmap-2"
+  }
+]
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmap-3", "node": "drive0", "persistent": true}}
+{"return": {}}
+bitmap info dump:
+[
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 16384,
+    "name": "bitmap-1"
+  },
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 32768,
+    "name": "bitmap-2"
+  },
+  {
+    "flags": [
+      "auto"
+    ],
+    "granularity": 65536,
+    "name": "bitmap-3"
+  }
+]
+
+Check "in-use" flag
+bitmap info dump:
+[
+  {
+    "flags": [
+      "in-use",
+      "auto"
+    ],
+    "granularity": 16384,
+    "name": "bitmap-1"
+  },
+  {
+    "flags": [
+      "in-use",
+      "auto"
+    ],
+    "granularity": 32768,
+    "name": "bitmap-2"
+  },
+  {
+    "flags": [
+      "in-use",
+      "auto"
+    ],
+    "granularity": 65536,
+    "name": "bitmap-3"
+  }
+]
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 v9 1/2] qemu-img info lists bitmap directory entries

[Eric Blake]

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

On 1/28/19 2:01 PM, 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:
> 

> 
> As the print of the qcow2 specific information expanded by adding
> the bitmap parameters to the 'qemu-img info' command output,
> it requires amendment of the output benchmark in the following
> tests: 060, 065, 082, 198, and 206.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---

>  
> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
> +{
> +    Qcow2BitmapInfoFlagsList *list = NULL;
> +    Qcow2BitmapInfoFlagsList **plist = &list;
> +
> +    if (flags & BME_FLAG_IN_USE) {
> +        Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
> +        entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
> +        *plist = entry;
> +        plist = &entry->next;

This line...

> +    }
> +    if (flags & BME_FLAG_AUTO) {
> +        Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
> +        entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
> +        *plist = entry;
> +    }

...is omitted here. Harmless for now, but may cause grief if a later
flag is added and we forget to add it in. On the other hand, I don't
know if a static analyzer might warn about a dead assignment, so
breaking the symmetry between the two is okay if that is the justification.

Also, thinking about future flag additions, would it make any sense to
write this code in a for loop?  Something like (untested):

    static const struct Map {
        int bme;
        int info;
    } map[] = {
        { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
        { BME_FLAG_AUTO,   QCOW2_BITMAP_INFO_FLAGS_AUTO },
    };

    for (i = 0; i < ARRAY_LENGTH(map); i++) {
        if (flags & map[i].bme) {
            ...; entry->value = map[i].info;
    }

where adding a new bit is now a one-liner change to the definition of
'map' rather than a 6-line addition of a new conditional.


> +##
> +# @Qcow2BitmapInfo:
> +#
> +# Qcow2 bitmap information.
> +#
> +# @name: the name of the bitmap
> +#
> +# @granularity: granularity of the bitmap in bytes
> +#
> +# @flags: flags of the bitmap
> +#
> +# @unknown-flags: unspecified flags if detected

Maybe:

@flags: recognized flags of the bitmap

@unknown-flags: any remaining flags not recognized by this qemu version


> +++ b/tests/qemu-iotests/060.out
> @@ -18,6 +18,7 @@ cluster_size: 65536
>  Format specific information:
>      compat: 1.1
>      lazy refcounts: false
> +    bitmaps:

Hmm. I'm wondering if the human-readable output of a QAPI type with an
optional array should output "<none>" or something similar for a
0-element array, to make it obvious to the human reading the output that
there are no bitmaps.  That's not necessarily a problem in your patch;
and may have even bigger effects to other iotests, so it should be done
as a separate patch if we want it.  But even in your patch, if we did
that,...

>      refcount bits: 16
>      corrupt: true
>  can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
> index 8bac383..86406cb 100755
> --- a/tests/qemu-iotests/065
> +++ b/tests/qemu-iotests/065
> @@ -88,23 +88,23 @@ class TestQMP(TestImageInfoSpecific):
>  class TestQCow2(TestQemuImgInfo):
>      '''Testing a qcow2 version 2 image'''
>      img_options = 'compat=0.10'
> -    json_compare = { 'compat': '0.10', 'refcount-bits': 16 }
> -    human_compare = [ 'compat: 0.10', 'refcount bits: 16' ]
> +    json_compare = { 'compat': '0.10', 'bitmaps': [], 'refcount-bits': 16 }
> +    human_compare = [ 'compat: 0.10', 'bitmaps:', 'refcount bits: 16' ]

...the human_compare dict would have to account for whatever string we
output for an empty JSON array.

I'm finding the functionality useful, though, so unless there are strong
opinions presented on making further tweaks, I'm also fine giving this
version as-is:

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 v9 1/2] qemu-img info lists bitmap directory entries

[Kevin Wolf]

Am 28.01.2019 um 21:01 hat Andrey Shinkevich geschrieben:
> 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
> 
> As the print of the qcow2 specific information expanded by adding
> the bitmap parameters to the 'qemu-img info' command output,
> it requires amendment of the output benchmark in the following
> tests: 060, 065, 082, 198, and 206.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  block/qapi.c               |  6 +++++
>  block/qcow2-bitmap.c       | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c              | 13 ++++++++++
>  block/qcow2.h              |  2 ++
>  qapi/block-core.json       | 42 +++++++++++++++++++++++++++++-
>  tests/qemu-iotests/060.out |  1 +
>  tests/qemu-iotests/065     | 16 ++++++------
>  tests/qemu-iotests/082.out |  7 +++++
>  tests/qemu-iotests/198.out |  2 ++
>  tests/qemu-iotests/206.out |  5 ++++
>  10 files changed, 149 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949..0fde98c 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -38,6 +38,7 @@
>  #include "qapi/qmp/qstring.h"
>  #include "sysemu/block-backend.h"
>  #include "qemu/cutils.h"
> +#include "qemu/error-report.h"
>  
>  BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>                                          BlockDriverState *bs, Error **errp)
> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>  
>      if (info->has_format_specific) {
>          func_fprintf(f, "Format specific information:\n");
> +        if (info->format_specific &&
> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
> +            warn_report("Failed to load bitmap list");
> +        }
>          bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>      }
>  }

Is it really necessary to introduce qcow2 specific knowledge in
block/qapi.c (where it definitely doesn't belong), just to emit a
warning?

Why can't you print the warning in qcow2_get_specific_info()?

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4897aba..07b99ee 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>          *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>              .compat             = g_strdup("0.10"),
>              .refcount_bits      = s->refcount_bits,
> +            .has_bitmaps        = true, /* To handle error check properly */
> +            .bitmaps            = NULL, /* Unsupported for version 2 */

.has_bitmaps = false would be nicer if the format doesn't even support
bitmaps. You only need this here because you put the warning in the
wrong place.

>          };
>      } else if (s->qcow_version == 3) {
> +        Qcow2BitmapInfoList *bitmaps;
> +        Error *local_err = NULL;
> +
> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);

Here you even had a good error message to print...

>          *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>              .compat             = g_strdup("1.1"),
>              .lazy_refcounts     = s->compatible_features &
> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>                                    QCOW2_INCOMPAT_CORRUPT,
>              .has_corrupt        = true,
>              .refcount_bits      = s->refcount_bits,
> +            .has_bitmaps        = !local_err,
> +            .bitmaps            = bitmaps,
>          };
> +        /*
> +         * If an error occurs in obtaining bitmaps, ignore
> +         * it to show other QCOW2 specific information.
> +         */
> +        error_free(local_err);

...but you decided to discard it.

How about you do this here:

    warn_reportf_err(local_err, "Failed to load bitmap list: ");

And then get rid of the code in block/qapi.c and the version 2 path?

Actually, should this really only be a warning, or in fact an error?
What's the case where we expect that loading the bitmap list can fail,
but the management tool doesn't need to know that and we just want to
log a message?

>      } else {
>          /* if this assertion fails, this probably means a new version was
>           * added without having it covered here */

Kevin

Re: [Qemu-devel] [PATCH v9 2/2] qemu-img info: bitmaps extension new test 239

[Kevin Wolf]

Am 28.01.2019 um 21:01 hat Andrey Shinkevich geschrieben:
> 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>

Can you add human output to the test, too?

Patch 1 changes a few reference outputs that change, including human
output, but they are all without bitmaps. I think we want at least one
test that tests human output with a non-empty list.

Kevin

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

[Andrey Shinkevich]

>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index c66f949..0fde98c 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -38,6 +38,7 @@
>>   #include "qapi/qmp/qstring.h"
>>   #include "sysemu/block-backend.h"
>>   #include "qemu/cutils.h"
>> +#include "qemu/error-report.h"
>>   
>>   BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>                                           BlockDriverState *bs, Error **errp)
>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>   
>>       if (info->has_format_specific) {
>>           func_fprintf(f, "Format specific information:\n");
>> +        if (info->format_specific &&
>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>> +            warn_report("Failed to load bitmap list");
>> +        }
>>           bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>       }
>>   }
> 
> Is it really necessary to introduce qcow2 specific knowledge in
> block/qapi.c (where it definitely doesn't belong), just to emit a
> warning?
> 
> Why can't you print the warning in qcow2_get_specific_info()?

Dear Kevin,
The reason behind the idea to move the warning to qapi is that we 
wouldn't like to print that in case of JSON format.

> 
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 4897aba..07b99ee 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>               .compat             = g_strdup("0.10"),
>>               .refcount_bits      = s->refcount_bits,
>> +            .has_bitmaps        = true, /* To handle error check properly */
>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
> 
> .has_bitmaps = false would be nicer if the format doesn't even support
> bitmaps. You only need this here because you put the warning in the
> wrong place.
> 
>>           };
>>       } else if (s->qcow_version == 3) {
>> +        Qcow2BitmapInfoList *bitmaps;
>> +        Error *local_err = NULL;
>> +
>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> 
> Here you even had a good error message to print...
> 
>>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>               .compat             = g_strdup("1.1"),
>>               .lazy_refcounts     = s->compatible_features &
>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>                                     QCOW2_INCOMPAT_CORRUPT,
>>               .has_corrupt        = true,
>>               .refcount_bits      = s->refcount_bits,
>> +            .has_bitmaps        = !local_err,
>> +            .bitmaps            = bitmaps,
>>           };
>> +        /*
>> +         * If an error occurs in obtaining bitmaps, ignore
>> +         * it to show other QCOW2 specific information.
>> +         */
>> +        error_free(local_err);
> 
> ...but you decided to discard it.
> 
> How about you do this here:
> 
>      warn_reportf_err(local_err, "Failed to load bitmap list: ");

In case of JSON format, we can fail here.
It will happen in the current implementation of the new test #239.

> 
> And then get rid of the code in block/qapi.c and the version 2 path?
> 
> Actually, should this really only be a warning, or in fact an error?
> What's the case where we expect that loading the bitmap list can fail,
> but the management tool doesn't need to know that and we just want to
> log a message?
> 
>>       } else {
>>           /* if this assertion fails, this probably means a new version was
>>            * added without having it covered here */
> 
> Kevin
> 

-- 
With the best regards,
Andrey Shinkevich

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

[Kevin Wolf]

Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >>
> >> diff --git a/block/qapi.c b/block/qapi.c
> >> index c66f949..0fde98c 100644
> >> --- a/block/qapi.c
> >> +++ b/block/qapi.c
> >> @@ -38,6 +38,7 @@
> >>   #include "qapi/qmp/qstring.h"
> >>   #include "sysemu/block-backend.h"
> >>   #include "qemu/cutils.h"
> >> +#include "qemu/error-report.h"
> >>   
> >>   BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> >>                                           BlockDriverState *bs, Error **errp)
> >> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> >>   
> >>       if (info->has_format_specific) {
> >>           func_fprintf(f, "Format specific information:\n");
> >> +        if (info->format_specific &&
> >> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
> >> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
> >> +            warn_report("Failed to load bitmap list");
> >> +        }
> >>           bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
> >>       }
> >>   }
> > 
> > Is it really necessary to introduce qcow2 specific knowledge in
> > block/qapi.c (where it definitely doesn't belong), just to emit a
> > warning?
> > 
> > Why can't you print the warning in qcow2_get_specific_info()?
> 
> Dear Kevin,
> The reason behind the idea to move the warning to qapi is that we 
> wouldn't like to print that in case of JSON format.
> 
> > 
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index 4897aba..07b99ee 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>               .compat             = g_strdup("0.10"),
> >>               .refcount_bits      = s->refcount_bits,
> >> +            .has_bitmaps        = true, /* To handle error check properly */
> >> +            .bitmaps            = NULL, /* Unsupported for version 2 */
> > 
> > .has_bitmaps = false would be nicer if the format doesn't even support
> > bitmaps. You only need this here because you put the warning in the
> > wrong place.
> > 
> >>           };
> >>       } else if (s->qcow_version == 3) {
> >> +        Qcow2BitmapInfoList *bitmaps;
> >> +        Error *local_err = NULL;
> >> +
> >> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> > 
> > Here you even had a good error message to print...
> > 
> >>           *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>               .compat             = g_strdup("1.1"),
> >>               .lazy_refcounts     = s->compatible_features &
> >> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>                                     QCOW2_INCOMPAT_CORRUPT,
> >>               .has_corrupt        = true,
> >>               .refcount_bits      = s->refcount_bits,
> >> +            .has_bitmaps        = !local_err,
> >> +            .bitmaps            = bitmaps,
> >>           };
> >> +        /*
> >> +         * If an error occurs in obtaining bitmaps, ignore
> >> +         * it to show other QCOW2 specific information.
> >> +         */
> >> +        error_free(local_err);
> > 
> > ...but you decided to discard it.
> > 
> > How about you do this here:
> > 
> >      warn_reportf_err(local_err, "Failed to load bitmap list: ");
> 
> In case of JSON format, we can fail here.
> It will happen in the current implementation of the new test #239.

Why do you want to silently leave out the bitmap list for JSON output?

Essentially, this is the same question I asked here:

> > And then get rid of the code in block/qapi.c and the version 2 path?
> > 
> > Actually, should this really only be a warning, or in fact an error?
> > What's the case where we expect that loading the bitmap list can fail,
> > but the management tool doesn't need to know that and we just want to
> > log a message?

I don't understand why it's okay not to print bitmaps without making it
an error. Do you have a specific use case in mind for this behaviour?

Kevin

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

[Vladimir Sementsov-Ogievskiy]

29.01.2019 15:38, Kevin Wolf wrote:
> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>
>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>> index c66f949..0fde98c 100644
>>>> --- a/block/qapi.c
>>>> +++ b/block/qapi.c
>>>> @@ -38,6 +38,7 @@
>>>>    #include "qapi/qmp/qstring.h"
>>>>    #include "sysemu/block-backend.h"
>>>>    #include "qemu/cutils.h"
>>>> +#include "qemu/error-report.h"
>>>>    
>>>>    BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>                                            BlockDriverState *bs, Error **errp)
>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>>>    
>>>>        if (info->has_format_specific) {
>>>>            func_fprintf(f, "Format specific information:\n");
>>>> +        if (info->format_specific &&
>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>>>> +            warn_report("Failed to load bitmap list");
>>>> +        }
>>>>            bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>>>        }
>>>>    }
>>>
>>> Is it really necessary to introduce qcow2 specific knowledge in
>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>> warning?
>>>
>>> Why can't you print the warning in qcow2_get_specific_info()?
>>
>> Dear Kevin,
>> The reason behind the idea to move the warning to qapi is that we
>> wouldn't like to print that in case of JSON format.
>>
>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 4897aba..07b99ee 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>            *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>                .compat             = g_strdup("0.10"),
>>>>                .refcount_bits      = s->refcount_bits,
>>>> +            .has_bitmaps        = true, /* To handle error check properly */
>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
>>>
>>> .has_bitmaps = false would be nicer if the format doesn't even support
>>> bitmaps. You only need this here because you put the warning in the
>>> wrong place.
>>>
>>>>            };
>>>>        } else if (s->qcow_version == 3) {
>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>> +        Error *local_err = NULL;
>>>> +
>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>
>>> Here you even had a good error message to print...
>>>
>>>>            *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>                .compat             = g_strdup("1.1"),
>>>>                .lazy_refcounts     = s->compatible_features &
>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>                                      QCOW2_INCOMPAT_CORRUPT,
>>>>                .has_corrupt        = true,
>>>>                .refcount_bits      = s->refcount_bits,
>>>> +            .has_bitmaps        = !local_err,
>>>> +            .bitmaps            = bitmaps,
>>>>            };
>>>> +        /*
>>>> +         * If an error occurs in obtaining bitmaps, ignore
>>>> +         * it to show other QCOW2 specific information.
>>>> +         */
>>>> +        error_free(local_err);
>>>
>>> ...but you decided to discard it.
>>>
>>> How about you do this here:
>>>
>>>       warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>
>> In case of JSON format, we can fail here.
>> It will happen in the current implementation of the new test #239.
> 
> Why do you want to silently leave out the bitmap list for JSON output?
> 
> Essentially, this is the same question I asked here:
> 
>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>
>>> Actually, should this really only be a warning, or in fact an error?
>>> What's the case where we expect that loading the bitmap list can fail,
>>> but the management tool doesn't need to know that and we just want to
>>> log a message?
> 
> I don't understand why it's okay not to print bitmaps without making it
> an error. Do you have a specific use case in mind for this behaviour?
> 


We just can't print anything here, as this code is executed from qmp command.

It was discussed, that we don't want to fail the whole query, if failed to
obtain bitmaps. So, to show, that there were error during bitmaps querying
we decided to use the following semantics:

empty list (has_bitmaps=true) means that there no bitmaps, and there were
no errors during bitmaps quering (if it was)

absence of the field (has_bitmaps=false) means that there _were_ errors during
bitmaps querying.

So qapi user, as well as json-output user should follow this semantics in
understanding the output. And as well as for qapi, it's incorrect to add text
messages to json output.

On the other hand, it seems reasonable to put additional notice for human-format
user.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

29.01.2019 15:50, Vladimir Sementsov-Ogievskiy wrote:
> 29.01.2019 15:38, Kevin Wolf wrote:
>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>>
>>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>>> index c66f949..0fde98c 100644
>>>>> --- a/block/qapi.c
>>>>> +++ b/block/qapi.c
>>>>> @@ -38,6 +38,7 @@
>>>>>    #include "qapi/qmp/qstring.h"
>>>>>    #include "sysemu/block-backend.h"
>>>>>    #include "qemu/cutils.h"
>>>>> +#include "qemu/error-report.h"
>>>>>    BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>                                            BlockDriverState *bs, Error **errp)
>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>>>>        if (info->has_format_specific) {
>>>>>            func_fprintf(f, "Format specific information:\n");
>>>>> +        if (info->format_specific &&
>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>>>>> +            warn_report("Failed to load bitmap list");
>>>>> +        }
>>>>>            bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>>>>        }
>>>>>    }
>>>>
>>>> Is it really necessary to introduce qcow2 specific knowledge in
>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>>> warning?
>>>>
>>>> Why can't you print the warning in qcow2_get_specific_info()?
>>>
>>> Dear Kevin,
>>> The reason behind the idea to move the warning to qapi is that we
>>> wouldn't like to print that in case of JSON format.
>>>
>>>>
>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>> index 4897aba..07b99ee 100644
>>>>> --- a/block/qcow2.c
>>>>> +++ b/block/qcow2.c
>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>            *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>                .compat             = g_strdup("0.10"),
>>>>>                .refcount_bits      = s->refcount_bits,
>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
>>>>
>>>> .has_bitmaps = false would be nicer if the format doesn't even support
>>>> bitmaps. You only need this here because you put the warning in the
>>>> wrong place.
>>>>
>>>>>            };
>>>>>        } else if (s->qcow_version == 3) {
>>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>>> +        Error *local_err = NULL;
>>>>> +
>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>
>>>> Here you even had a good error message to print...
>>>>
>>>>>            *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>                .compat             = g_strdup("1.1"),
>>>>>                .lazy_refcounts     = s->compatible_features &
>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>                                      QCOW2_INCOMPAT_CORRUPT,
>>>>>                .has_corrupt        = true,
>>>>>                .refcount_bits      = s->refcount_bits,
>>>>> +            .has_bitmaps        = !local_err,
>>>>> +            .bitmaps            = bitmaps,
>>>>>            };
>>>>> +        /*
>>>>> +         * If an error occurs in obtaining bitmaps, ignore
>>>>> +         * it to show other QCOW2 specific information.
>>>>> +         */
>>>>> +        error_free(local_err);
>>>>
>>>> ...but you decided to discard it.
>>>>
>>>> How about you do this here:
>>>>
>>>>       warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>>
>>> In case of JSON format, we can fail here.
>>> It will happen in the current implementation of the new test #239.
>>
>> Why do you want to silently leave out the bitmap list for JSON output?
>>
>> Essentially, this is the same question I asked here:
>>
>>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>>
>>>> Actually, should this really only be a warning, or in fact an error?
>>>> What's the case where we expect that loading the bitmap list can fail,
>>>> but the management tool doesn't need to know that and we just want to
>>>> log a message?
>>
>> I don't understand why it's okay not to print bitmaps without making it
>> an error. Do you have a specific use case in mind for this behaviour?
>>
> 
> 
> We just can't print anything here, as this code is executed from qmp command.

Hmm, or we can?

Ok, for json output, it may be ok to print warnings to stderr, while formatted
json to stdout.

And for qapi, warning will go to logs, which is not bad too.

Ok, now I don't see any counterarguments..


-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 15:38, Kevin Wolf wrote:
> > Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >>>>
> >>>> diff --git a/block/qapi.c b/block/qapi.c
> >>>> index c66f949..0fde98c 100644
> >>>> --- a/block/qapi.c
> >>>> +++ b/block/qapi.c
> >>>> @@ -38,6 +38,7 @@
> >>>>    #include "qapi/qmp/qstring.h"
> >>>>    #include "sysemu/block-backend.h"
> >>>>    #include "qemu/cutils.h"
> >>>> +#include "qemu/error-report.h"
> >>>>    
> >>>>    BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> >>>>                                            BlockDriverState *bs, Error **errp)
> >>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> >>>>    
> >>>>        if (info->has_format_specific) {
> >>>>            func_fprintf(f, "Format specific information:\n");
> >>>> +        if (info->format_specific &&
> >>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
> >>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
> >>>> +            warn_report("Failed to load bitmap list");
> >>>> +        }
> >>>>            bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
> >>>>        }
> >>>>    }
> >>>
> >>> Is it really necessary to introduce qcow2 specific knowledge in
> >>> block/qapi.c (where it definitely doesn't belong), just to emit a
> >>> warning?
> >>>
> >>> Why can't you print the warning in qcow2_get_specific_info()?
> >>
> >> Dear Kevin,
> >> The reason behind the idea to move the warning to qapi is that we
> >> wouldn't like to print that in case of JSON format.
> >>
> >>>
> >>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>> index 4897aba..07b99ee 100644
> >>>> --- a/block/qcow2.c
> >>>> +++ b/block/qcow2.c
> >>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>>>            *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>>>                .compat             = g_strdup("0.10"),
> >>>>                .refcount_bits      = s->refcount_bits,
> >>>> +            .has_bitmaps        = true, /* To handle error check properly */
> >>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
> >>>
> >>> .has_bitmaps = false would be nicer if the format doesn't even support
> >>> bitmaps. You only need this here because you put the warning in the
> >>> wrong place.
> >>>
> >>>>            };
> >>>>        } else if (s->qcow_version == 3) {
> >>>> +        Qcow2BitmapInfoList *bitmaps;
> >>>> +        Error *local_err = NULL;
> >>>> +
> >>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> >>>
> >>> Here you even had a good error message to print...
> >>>
> >>>>            *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>>>                .compat             = g_strdup("1.1"),
> >>>>                .lazy_refcounts     = s->compatible_features &
> >>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>>>                                      QCOW2_INCOMPAT_CORRUPT,
> >>>>                .has_corrupt        = true,
> >>>>                .refcount_bits      = s->refcount_bits,
> >>>> +            .has_bitmaps        = !local_err,
> >>>> +            .bitmaps            = bitmaps,
> >>>>            };
> >>>> +        /*
> >>>> +         * If an error occurs in obtaining bitmaps, ignore
> >>>> +         * it to show other QCOW2 specific information.
> >>>> +         */
> >>>> +        error_free(local_err);
> >>>
> >>> ...but you decided to discard it.
> >>>
> >>> How about you do this here:
> >>>
> >>>       warn_reportf_err(local_err, "Failed to load bitmap list: ");
> >>
> >> In case of JSON format, we can fail here.
> >> It will happen in the current implementation of the new test #239.
> > 
> > Why do you want to silently leave out the bitmap list for JSON output?
> > 
> > Essentially, this is the same question I asked here:
> > 
> >>> And then get rid of the code in block/qapi.c and the version 2 path?
> >>>
> >>> Actually, should this really only be a warning, or in fact an error?
> >>> What's the case where we expect that loading the bitmap list can fail,
> >>> but the management tool doesn't need to know that and we just want to
> >>> log a message?
> > 
> > I don't understand why it's okay not to print bitmaps without making it
> > an error. Do you have a specific use case in mind for this behaviour?
> > 
> 
> 
> We just can't print anything here, as this code is executed from qmp command.
> 
> It was discussed, that we don't want to fail the whole query, if failed to
> obtain bitmaps.

It's obvious that you don't want to fail the query command, but I
haven't found any explanation for _why_ you want this.

The thing that is closest to an explanation is "it may be sad to fail
get any information because of repeating some disk/qcow2 error", written
by you in the v4 thread.

I don't think "may be sad" is a good justification for non-standard
behaviour. If we can't load the bitmaps, the image is broken. And if the
image is broken, the rest of the information may be invalid, too.

> So, to show, that there were error during bitmaps querying
> we decided to use the following semantics:
> 
> empty list (has_bitmaps=true) means that there no bitmaps, and there were
> no errors during bitmaps quering (if it was)
> 
> absence of the field (has_bitmaps=false) means that there _were_ errors during
> bitmaps querying.

...or that you're running an old QEMU version. I'm not sure if making
old QEMU versions and image errors look the same is a good move.

Almost worse than making the QAPI interface confusing, however, is that
we throw away the real error message without giving it to the user.

> So qapi user, as well as json-output user should follow this semantics in
> understanding the output. And as well as for qapi, it's incorrect to add text
> messages to json output.
> 
> On the other hand, it seems reasonable to put additional notice for human-format
> user.

Even for QMP, a log message on stderr is better than nothing.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

29.01.2019 16:17, Kevin Wolf wrote:
> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 15:38, Kevin Wolf wrote:
>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>>>
>>>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>>>> index c66f949..0fde98c 100644
>>>>>> --- a/block/qapi.c
>>>>>> +++ b/block/qapi.c
>>>>>> @@ -38,6 +38,7 @@
>>>>>>     #include "qapi/qmp/qstring.h"
>>>>>>     #include "sysemu/block-backend.h"
>>>>>>     #include "qemu/cutils.h"
>>>>>> +#include "qemu/error-report.h"
>>>>>>     
>>>>>>     BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>                                             BlockDriverState *bs, Error **errp)
>>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>>>>>     
>>>>>>         if (info->has_format_specific) {
>>>>>>             func_fprintf(f, "Format specific information:\n");
>>>>>> +        if (info->format_specific &&
>>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>>>>>> +            warn_report("Failed to load bitmap list");
>>>>>> +        }
>>>>>>             bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>>>>>         }
>>>>>>     }
>>>>>
>>>>> Is it really necessary to introduce qcow2 specific knowledge in
>>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>>>> warning?
>>>>>
>>>>> Why can't you print the warning in qcow2_get_specific_info()?
>>>>
>>>> Dear Kevin,
>>>> The reason behind the idea to move the warning to qapi is that we
>>>> wouldn't like to print that in case of JSON format.
>>>>
>>>>>
>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>> index 4897aba..07b99ee 100644
>>>>>> --- a/block/qcow2.c
>>>>>> +++ b/block/qcow2.c
>>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>             *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>                 .compat             = g_strdup("0.10"),
>>>>>>                 .refcount_bits      = s->refcount_bits,
>>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
>>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
>>>>>
>>>>> .has_bitmaps = false would be nicer if the format doesn't even support
>>>>> bitmaps. You only need this here because you put the warning in the
>>>>> wrong place.
>>>>>
>>>>>>             };
>>>>>>         } else if (s->qcow_version == 3) {
>>>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>>>> +        Error *local_err = NULL;
>>>>>> +
>>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>>
>>>>> Here you even had a good error message to print...
>>>>>
>>>>>>             *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>                 .compat             = g_strdup("1.1"),
>>>>>>                 .lazy_refcounts     = s->compatible_features &
>>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>                                       QCOW2_INCOMPAT_CORRUPT,
>>>>>>                 .has_corrupt        = true,
>>>>>>                 .refcount_bits      = s->refcount_bits,
>>>>>> +            .has_bitmaps        = !local_err,
>>>>>> +            .bitmaps            = bitmaps,
>>>>>>             };
>>>>>> +        /*
>>>>>> +         * If an error occurs in obtaining bitmaps, ignore
>>>>>> +         * it to show other QCOW2 specific information.
>>>>>> +         */
>>>>>> +        error_free(local_err);
>>>>>
>>>>> ...but you decided to discard it.
>>>>>
>>>>> How about you do this here:
>>>>>
>>>>>        warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>>>
>>>> In case of JSON format, we can fail here.
>>>> It will happen in the current implementation of the new test #239.
>>>
>>> Why do you want to silently leave out the bitmap list for JSON output?
>>>
>>> Essentially, this is the same question I asked here:
>>>
>>>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>>>
>>>>> Actually, should this really only be a warning, or in fact an error?
>>>>> What's the case where we expect that loading the bitmap list can fail,
>>>>> but the management tool doesn't need to know that and we just want to
>>>>> log a message?
>>>
>>> I don't understand why it's okay not to print bitmaps without making it
>>> an error. Do you have a specific use case in mind for this behaviour?
>>>
>>
>>
>> We just can't print anything here, as this code is executed from qmp command.
>>
>> It was discussed, that we don't want to fail the whole query, if failed to
>> obtain bitmaps.
> 
> It's obvious that you don't want to fail the query command, but I
> haven't found any explanation for _why_ you want this.
> 
> The thing that is closest to an explanation is "it may be sad to fail
> get any information because of repeating some disk/qcow2 error", written
> by you in the v4 thread.
> 
> I don't think "may be sad" is a good justification for non-standard
> behaviour. If we can't load the bitmaps, the image is broken. And if the
> image is broken, the rest of the information may be invalid, too.

So, you mean that we go wrong way. And it was my idea. That is sad too.

Than, seems like we should add errp to the function and to the full stack
down to qmp_query_block. And drop extra partial-success qapi logic.

> 
>> So, to show, that there were error during bitmaps querying
>> we decided to use the following semantics:
>>
>> empty list (has_bitmaps=true) means that there no bitmaps, and there were
>> no errors during bitmaps quering (if it was)
>>
>> absence of the field (has_bitmaps=false) means that there _were_ errors during
>> bitmaps querying.
> 
> ...or that you're running an old QEMU version. I'm not sure if making
> old QEMU versions and image errors look the same is a good move.

No, for old versions we return empty list, to show that there were no errors.

> 
> Almost worse than making the QAPI interface confusing, however, is that
> we throw away the real error message without giving it to the user.
> 
>> So qapi user, as well as json-output user should follow this semantics in
>> understanding the output. And as well as for qapi, it's incorrect to add text
>> messages to json output.
>>
>> On the other hand, it seems reasonable to put additional notice for human-format
>> user.
> 
> Even for QMP, a log message on stderr is better than nothing.
> 

Agreed.

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 16:17, Kevin Wolf wrote:
> > Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 29.01.2019 15:38, Kevin Wolf wrote:
> >>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >>>>>>
> >>>>>> diff --git a/block/qapi.c b/block/qapi.c
> >>>>>> index c66f949..0fde98c 100644
> >>>>>> --- a/block/qapi.c
> >>>>>> +++ b/block/qapi.c
> >>>>>> @@ -38,6 +38,7 @@
> >>>>>>     #include "qapi/qmp/qstring.h"
> >>>>>>     #include "sysemu/block-backend.h"
> >>>>>>     #include "qemu/cutils.h"
> >>>>>> +#include "qemu/error-report.h"
> >>>>>>     
> >>>>>>     BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> >>>>>>                                             BlockDriverState *bs, Error **errp)
> >>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> >>>>>>     
> >>>>>>         if (info->has_format_specific) {
> >>>>>>             func_fprintf(f, "Format specific information:\n");
> >>>>>> +        if (info->format_specific &&
> >>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
> >>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
> >>>>>> +            warn_report("Failed to load bitmap list");
> >>>>>> +        }
> >>>>>>             bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
> >>>>>>         }
> >>>>>>     }
> >>>>>
> >>>>> Is it really necessary to introduce qcow2 specific knowledge in
> >>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
> >>>>> warning?
> >>>>>
> >>>>> Why can't you print the warning in qcow2_get_specific_info()?
> >>>>
> >>>> Dear Kevin,
> >>>> The reason behind the idea to move the warning to qapi is that we
> >>>> wouldn't like to print that in case of JSON format.
> >>>>
> >>>>>
> >>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>>> index 4897aba..07b99ee 100644
> >>>>>> --- a/block/qcow2.c
> >>>>>> +++ b/block/qcow2.c
> >>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>>>>>             *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>>>>>                 .compat             = g_strdup("0.10"),
> >>>>>>                 .refcount_bits      = s->refcount_bits,
> >>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
> >>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
> >>>>>
> >>>>> .has_bitmaps = false would be nicer if the format doesn't even support
> >>>>> bitmaps. You only need this here because you put the warning in the
> >>>>> wrong place.
> >>>>>
> >>>>>>             };
> >>>>>>         } else if (s->qcow_version == 3) {
> >>>>>> +        Qcow2BitmapInfoList *bitmaps;
> >>>>>> +        Error *local_err = NULL;
> >>>>>> +
> >>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> >>>>>
> >>>>> Here you even had a good error message to print...
> >>>>>
> >>>>>>             *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>>>>>                 .compat             = g_strdup("1.1"),
> >>>>>>                 .lazy_refcounts     = s->compatible_features &
> >>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>>>>>                                       QCOW2_INCOMPAT_CORRUPT,
> >>>>>>                 .has_corrupt        = true,
> >>>>>>                 .refcount_bits      = s->refcount_bits,
> >>>>>> +            .has_bitmaps        = !local_err,
> >>>>>> +            .bitmaps            = bitmaps,
> >>>>>>             };
> >>>>>> +        /*
> >>>>>> +         * If an error occurs in obtaining bitmaps, ignore
> >>>>>> +         * it to show other QCOW2 specific information.
> >>>>>> +         */
> >>>>>> +        error_free(local_err);
> >>>>>
> >>>>> ...but you decided to discard it.
> >>>>>
> >>>>> How about you do this here:
> >>>>>
> >>>>>        warn_reportf_err(local_err, "Failed to load bitmap list: ");
> >>>>
> >>>> In case of JSON format, we can fail here.
> >>>> It will happen in the current implementation of the new test #239.
> >>>
> >>> Why do you want to silently leave out the bitmap list for JSON output?
> >>>
> >>> Essentially, this is the same question I asked here:
> >>>
> >>>>> And then get rid of the code in block/qapi.c and the version 2 path?
> >>>>>
> >>>>> Actually, should this really only be a warning, or in fact an error?
> >>>>> What's the case where we expect that loading the bitmap list can fail,
> >>>>> but the management tool doesn't need to know that and we just want to
> >>>>> log a message?
> >>>
> >>> I don't understand why it's okay not to print bitmaps without making it
> >>> an error. Do you have a specific use case in mind for this behaviour?
> >>>
> >>
> >>
> >> We just can't print anything here, as this code is executed from qmp command.
> >>
> >> It was discussed, that we don't want to fail the whole query, if failed to
> >> obtain bitmaps.
> > 
> > It's obvious that you don't want to fail the query command, but I
> > haven't found any explanation for _why_ you want this.
> > 
> > The thing that is closest to an explanation is "it may be sad to fail
> > get any information because of repeating some disk/qcow2 error", written
> > by you in the v4 thread.
> > 
> > I don't think "may be sad" is a good justification for non-standard
> > behaviour. If we can't load the bitmaps, the image is broken. And if the
> > image is broken, the rest of the information may be invalid, too.
> 
> So, you mean that we go wrong way. And it was my idea. That is sad too.
> 
> Than, seems like we should add errp to the function and to the full stack
> down to qmp_query_block. And drop extra partial-success qapi logic.

I'm not necessarily saying that it's the wrong way, though possibly it
is wrong indeed.

But ignoring some kind of failures is special, so what I was looking for
were comments or documentation to explain the reason behind it at least.
Now from the replies I got so far it looks to me that possibly the
reasons are not only undocumented, but we might actually not have any.

> >> So, to show, that there were error during bitmaps querying
> >> we decided to use the following semantics:
> >>
> >> empty list (has_bitmaps=true) means that there no bitmaps, and there were
> >> no errors during bitmaps quering (if it was)
> >>
> >> absence of the field (has_bitmaps=false) means that there _were_ errors during
> >> bitmaps querying.
> > 
> > ...or that you're running an old QEMU version. I'm not sure if making
> > old QEMU versions and image errors look the same is a good move.
> 
> No, for old versions we return empty list, to show that there were no errors.

You mean old image format versions? I'm talking about old QEMU versions
that don't even know the 'bitmaps' field.

A QMP client would have to parse the schema to understand whether a
missing 'bitmaps' field means that it's talking to an old QEMU (then
'bitmaps' doesn't exist in the schema), or that an error happened (then
the field does exist in the schema).

Looking at the schema is not impossible, so if we require this for a
good reason, it's okay. But it's not trivial either. If we really want
to go with this semantics, we should probably mention both the old and
the new meaning in the QAPI documentation, with the recommendation that
you parse the schema to determine the meaning of a missing 'bitmaps'
field.

Kevin

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

[Vladimir Sementsov-Ogievskiy]

29.01.2019 17:23, Kevin Wolf wrote:
> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 16:17, Kevin Wolf wrote:
>>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 29.01.2019 15:38, Kevin Wolf wrote:
>>>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>>>>>
>>>>>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>>>>>> index c66f949..0fde98c 100644
>>>>>>>> --- a/block/qapi.c
>>>>>>>> +++ b/block/qapi.c
>>>>>>>> @@ -38,6 +38,7 @@
>>>>>>>>      #include "qapi/qmp/qstring.h"
>>>>>>>>      #include "sysemu/block-backend.h"
>>>>>>>>      #include "qemu/cutils.h"
>>>>>>>> +#include "qemu/error-report.h"
>>>>>>>>      
>>>>>>>>      BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>>>                                              BlockDriverState *bs, Error **errp)
>>>>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>>>>>>>      
>>>>>>>>          if (info->has_format_specific) {
>>>>>>>>              func_fprintf(f, "Format specific information:\n");
>>>>>>>> +        if (info->format_specific &&
>>>>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>>>>>>>> +            warn_report("Failed to load bitmap list");
>>>>>>>> +        }
>>>>>>>>              bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>>>>>>>          }
>>>>>>>>      }
>>>>>>>
>>>>>>> Is it really necessary to introduce qcow2 specific knowledge in
>>>>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>>>>>> warning?
>>>>>>>
>>>>>>> Why can't you print the warning in qcow2_get_specific_info()?
>>>>>>
>>>>>> Dear Kevin,
>>>>>> The reason behind the idea to move the warning to qapi is that we
>>>>>> wouldn't like to print that in case of JSON format.
>>>>>>
>>>>>>>
>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>> index 4897aba..07b99ee 100644
>>>>>>>> --- a/block/qcow2.c
>>>>>>>> +++ b/block/qcow2.c
>>>>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>              *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>                  .compat             = g_strdup("0.10"),
>>>>>>>>                  .refcount_bits      = s->refcount_bits,
>>>>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
>>>>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
>>>>>>>
>>>>>>> .has_bitmaps = false would be nicer if the format doesn't even support
>>>>>>> bitmaps. You only need this here because you put the warning in the
>>>>>>> wrong place.
>>>>>>>
>>>>>>>>              };
>>>>>>>>          } else if (s->qcow_version == 3) {
>>>>>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>>>>>> +        Error *local_err = NULL;
>>>>>>>> +
>>>>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>>>>
>>>>>>> Here you even had a good error message to print...
>>>>>>>
>>>>>>>>              *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>                  .compat             = g_strdup("1.1"),
>>>>>>>>                  .lazy_refcounts     = s->compatible_features &
>>>>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>                                        QCOW2_INCOMPAT_CORRUPT,
>>>>>>>>                  .has_corrupt        = true,
>>>>>>>>                  .refcount_bits      = s->refcount_bits,
>>>>>>>> +            .has_bitmaps        = !local_err,
>>>>>>>> +            .bitmaps            = bitmaps,
>>>>>>>>              };
>>>>>>>> +        /*
>>>>>>>> +         * If an error occurs in obtaining bitmaps, ignore
>>>>>>>> +         * it to show other QCOW2 specific information.
>>>>>>>> +         */
>>>>>>>> +        error_free(local_err);
>>>>>>>
>>>>>>> ...but you decided to discard it.
>>>>>>>
>>>>>>> How about you do this here:
>>>>>>>
>>>>>>>         warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>>>>>
>>>>>> In case of JSON format, we can fail here.
>>>>>> It will happen in the current implementation of the new test #239.
>>>>>
>>>>> Why do you want to silently leave out the bitmap list for JSON output?
>>>>>
>>>>> Essentially, this is the same question I asked here:
>>>>>
>>>>>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>>>>>
>>>>>>> Actually, should this really only be a warning, or in fact an error?
>>>>>>> What's the case where we expect that loading the bitmap list can fail,
>>>>>>> but the management tool doesn't need to know that and we just want to
>>>>>>> log a message?
>>>>>
>>>>> I don't understand why it's okay not to print bitmaps without making it
>>>>> an error. Do you have a specific use case in mind for this behaviour?
>>>>>
>>>>
>>>>
>>>> We just can't print anything here, as this code is executed from qmp command.
>>>>
>>>> It was discussed, that we don't want to fail the whole query, if failed to
>>>> obtain bitmaps.
>>>
>>> It's obvious that you don't want to fail the query command, but I
>>> haven't found any explanation for _why_ you want this.
>>>
>>> The thing that is closest to an explanation is "it may be sad to fail
>>> get any information because of repeating some disk/qcow2 error", written
>>> by you in the v4 thread.
>>>
>>> I don't think "may be sad" is a good justification for non-standard
>>> behaviour. If we can't load the bitmaps, the image is broken. And if the
>>> image is broken, the rest of the information may be invalid, too.
>>
>> So, you mean that we go wrong way. And it was my idea. That is sad too.
>>
>> Than, seems like we should add errp to the function and to the full stack
>> down to qmp_query_block. And drop extra partial-success qapi logic.
> 
> I'm not necessarily saying that it's the wrong way, though possibly it
> is wrong indeed.
> 
> But ignoring some kind of failures is special, so what I was looking for
> were comments or documentation to explain the reason behind it at least.
> Now from the replies I got so far it looks to me that possibly the
> reasons are not only undocumented, but we might actually not have any.
> 
>>>> So, to show, that there were error during bitmaps querying
>>>> we decided to use the following semantics:
>>>>
>>>> empty list (has_bitmaps=true) means that there no bitmaps, and there were
>>>> no errors during bitmaps quering (if it was)
>>>>
>>>> absence of the field (has_bitmaps=false) means that there _were_ errors during
>>>> bitmaps querying.
>>>
>>> ...or that you're running an old QEMU version. I'm not sure if making
>>> old QEMU versions and image errors look the same is a good move.
>>
>> No, for old versions we return empty list, to show that there were no errors.
> 
> You mean old image format versions?

yes.

I'm talking about old QEMU versions
> that don't even know the 'bitmaps' field.

hmm. Yes, that's a problem, which we didn't considered.

> 
> A QMP client would have to parse the schema to understand whether a
> missing 'bitmaps' field means that it's talking to an old QEMU (then
> 'bitmaps' doesn't exist in the schema), or that an error happened (then
> the field does exist in the schema).
> 
> Looking at the schema is not impossible, so if we require this for a
> good reason, it's okay. But it's not trivial either. If we really want
> to go with this semantics, we should probably mention both the old and
> the new meaning in the QAPI documentation, with the recommendation that
> you parse the schema to determine the meaning of a missing 'bitmaps'
> field.

Hm, now I think that if we really face the case when we need partial success
of info querying, it should be additional optional parameter which enables it, like
skip-failed=true (default false) or something, which is more clear than parsing the
schema. And, which we can add later if needed.


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
> 29.01.2019 17:23, Kevin Wolf wrote:
>> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 29.01.2019 16:17, Kevin Wolf wrote:
>>>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> 29.01.2019 15:38, Kevin Wolf wrote:
>>>>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>>>>>>
>>>>>>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>>>>>>> index c66f949..0fde98c 100644
>>>>>>>>> --- a/block/qapi.c
>>>>>>>>> +++ b/block/qapi.c
>>>>>>>>> @@ -38,6 +38,7 @@
>>>>>>>>>      #include "qapi/qmp/qstring.h"
>>>>>>>>>      #include "sysemu/block-backend.h"
>>>>>>>>>      #include "qemu/cutils.h"
>>>>>>>>> +#include "qemu/error-report.h"
>>>>>>>>>      BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>>>>                                              BlockDriverState *bs, Error **errp)
>>>>>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>>>>>>>>          if (info->has_format_specific) {
>>>>>>>>>              func_fprintf(f, "Format specific information:\n");
>>>>>>>>> +        if (info->format_specific &&
>>>>>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>>>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>>>>>>>>> +            warn_report("Failed to load bitmap list");
>>>>>>>>> +        }
>>>>>>>>>              bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>>>>>>>>          }
>>>>>>>>>      }
>>>>>>>>
>>>>>>>> Is it really necessary to introduce qcow2 specific knowledge in
>>>>>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>>>>>>> warning?
>>>>>>>>
>>>>>>>> Why can't you print the warning in qcow2_get_specific_info()?
>>>>>>>
>>>>>>> Dear Kevin,
>>>>>>> The reason behind the idea to move the warning to qapi is that we
>>>>>>> wouldn't like to print that in case of JSON format.
>>>>>>>
>>>>>>>>
>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>> index 4897aba..07b99ee 100644
>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>>              *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>>                  .compat             = g_strdup("0.10"),
>>>>>>>>>                  .refcount_bits      = s->refcount_bits,
>>>>>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
>>>>>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
>>>>>>>>
>>>>>>>> .has_bitmaps = false would be nicer if the format doesn't even support
>>>>>>>> bitmaps. You only need this here because you put the warning in the
>>>>>>>> wrong place.
>>>>>>>>
>>>>>>>>>              };
>>>>>>>>>          } else if (s->qcow_version == 3) {
>>>>>>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>>>>>>> +        Error *local_err = NULL;
>>>>>>>>> +
>>>>>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>>>>>
>>>>>>>> Here you even had a good error message to print...
>>>>>>>>
>>>>>>>>>              *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>>                  .compat             = g_strdup("1.1"),
>>>>>>>>>                  .lazy_refcounts     = s->compatible_features &
>>>>>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>>                                        QCOW2_INCOMPAT_CORRUPT,
>>>>>>>>>                  .has_corrupt        = true,
>>>>>>>>>                  .refcount_bits      = s->refcount_bits,
>>>>>>>>> +            .has_bitmaps        = !local_err,
>>>>>>>>> +            .bitmaps            = bitmaps,
>>>>>>>>>              };
>>>>>>>>> +        /*
>>>>>>>>> +         * If an error occurs in obtaining bitmaps, ignore
>>>>>>>>> +         * it to show other QCOW2 specific information.
>>>>>>>>> +         */
>>>>>>>>> +        error_free(local_err);
>>>>>>>>
>>>>>>>> ...but you decided to discard it.
>>>>>>>>
>>>>>>>> How about you do this here:
>>>>>>>>
>>>>>>>>         warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>>>>>>
>>>>>>> In case of JSON format, we can fail here.
>>>>>>> It will happen in the current implementation of the new test #239.
>>>>>>
>>>>>> Why do you want to silently leave out the bitmap list for JSON output?
>>>>>>
>>>>>> Essentially, this is the same question I asked here:
>>>>>>
>>>>>>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>>>>>>
>>>>>>>> Actually, should this really only be a warning, or in fact an error?
>>>>>>>> What's the case where we expect that loading the bitmap list can fail,
>>>>>>>> but the management tool doesn't need to know that and we just want to
>>>>>>>> log a message?
>>>>>>
>>>>>> I don't understand why it's okay not to print bitmaps without making it
>>>>>> an error. Do you have a specific use case in mind for this behaviour?
>>>>>>
>>>>>
>>>>>
>>>>> We just can't print anything here, as this code is executed from qmp command.
>>>>>
>>>>> It was discussed, that we don't want to fail the whole query, if failed to
>>>>> obtain bitmaps.
>>>>
>>>> It's obvious that you don't want to fail the query command, but I
>>>> haven't found any explanation for _why_ you want this.
>>>>
>>>> The thing that is closest to an explanation is "it may be sad to fail
>>>> get any information because of repeating some disk/qcow2 error", written
>>>> by you in the v4 thread.
>>>>
>>>> I don't think "may be sad" is a good justification for non-standard
>>>> behaviour. If we can't load the bitmaps, the image is broken. And if the
>>>> image is broken, the rest of the information may be invalid, too.
>>>
>>> So, you mean that we go wrong way. And it was my idea. That is sad too.
>>>
>>> Than, seems like we should add errp to the function and to the full stack
>>> down to qmp_query_block. And drop extra partial-success qapi logic.
>>
>> I'm not necessarily saying that it's the wrong way, though possibly it
>> is wrong indeed.
>>
>> But ignoring some kind of failures is special, so what I was looking for
>> were comments or documentation to explain the reason behind it at least.
>> Now from the replies I got so far it looks to me that possibly the
>> reasons are not only undocumented, but we might actually not have any.
>>
>>>>> So, to show, that there were error during bitmaps querying
>>>>> we decided to use the following semantics:
>>>>>
>>>>> empty list (has_bitmaps=true) means that there no bitmaps, and there were
>>>>> no errors during bitmaps quering (if it was)
>>>>>
>>>>> absence of the field (has_bitmaps=false) means that there _were_ errors during
>>>>> bitmaps querying.
>>>>
>>>> ...or that you're running an old QEMU version. I'm not sure if making
>>>> old QEMU versions and image errors look the same is a good move.
>>>
>>> No, for old versions we return empty list, to show that there were no errors.
>>
>> You mean old image format versions?
> 
> yes.
> 
> I'm talking about old QEMU versions
>> that don't even know the 'bitmaps' field.
> 
> hmm. Yes, that's a problem, which we didn't considered.
> 
>>
>> A QMP client would have to parse the schema to understand whether a
>> missing 'bitmaps' field means that it's talking to an old QEMU (then
>> 'bitmaps' doesn't exist in the schema), or that an error happened (then
>> the field does exist in the schema).
>>
>> Looking at the schema is not impossible, so if we require this for a
>> good reason, it's okay. But it's not trivial either. If we really want
>> to go with this semantics, we should probably mention both the old and
>> the new meaning in the QAPI documentation, with the recommendation that
>> you parse the schema to determine the meaning of a missing 'bitmaps'
>> field.
> 
> Hm, now I think that if we really face the case when we need partial success
> of info querying, it should be additional optional parameter which enables it, like
> skip-failed=true (default false) or something, which is more clear than parsing the
> schema. And, which we can add later if needed.
> 
> 

Note also, that we already skip some errors, for example, block_crypto_get_specific_info_luks
will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and
bdrv_query_image_info skip these errors, in same manner as for formats which don't support
bdrv_get_specific_info.

Looks like a good moment to fix this too, do you agree?

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 29.01.2019 um 16:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
> > 29.01.2019 17:23, Kevin Wolf wrote:
> >> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>> 29.01.2019 16:17, Kevin Wolf wrote:
> >>>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>>> 29.01.2019 15:38, Kevin Wolf wrote:
> >>>>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
> >>>>>>>>>
> >>>>>>>>> diff --git a/block/qapi.c b/block/qapi.c
> >>>>>>>>> index c66f949..0fde98c 100644
> >>>>>>>>> --- a/block/qapi.c
> >>>>>>>>> +++ b/block/qapi.c
> >>>>>>>>> @@ -38,6 +38,7 @@
> >>>>>>>>>      #include "qapi/qmp/qstring.h"
> >>>>>>>>>      #include "sysemu/block-backend.h"
> >>>>>>>>>      #include "qemu/cutils.h"
> >>>>>>>>> +#include "qemu/error-report.h"
> >>>>>>>>>      BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
> >>>>>>>>>                                              BlockDriverState *bs, Error **errp)
> >>>>>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> >>>>>>>>>          if (info->has_format_specific) {
> >>>>>>>>>              func_fprintf(f, "Format specific information:\n");
> >>>>>>>>> +        if (info->format_specific &&
> >>>>>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
> >>>>>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
> >>>>>>>>> +            warn_report("Failed to load bitmap list");
> >>>>>>>>> +        }
> >>>>>>>>>              bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
> >>>>>>>>>          }
> >>>>>>>>>      }
> >>>>>>>>
> >>>>>>>> Is it really necessary to introduce qcow2 specific knowledge in
> >>>>>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
> >>>>>>>> warning?
> >>>>>>>>
> >>>>>>>> Why can't you print the warning in qcow2_get_specific_info()?
> >>>>>>>
> >>>>>>> Dear Kevin,
> >>>>>>> The reason behind the idea to move the warning to qapi is that we
> >>>>>>> wouldn't like to print that in case of JSON format.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>>>>>> index 4897aba..07b99ee 100644
> >>>>>>>>> --- a/block/qcow2.c
> >>>>>>>>> +++ b/block/qcow2.c
> >>>>>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>>>>>>>>              *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>>>>>>>>                  .compat             = g_strdup("0.10"),
> >>>>>>>>>                  .refcount_bits      = s->refcount_bits,
> >>>>>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
> >>>>>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
> >>>>>>>>
> >>>>>>>> .has_bitmaps = false would be nicer if the format doesn't even support
> >>>>>>>> bitmaps. You only need this here because you put the warning in the
> >>>>>>>> wrong place.
> >>>>>>>>
> >>>>>>>>>              };
> >>>>>>>>>          } else if (s->qcow_version == 3) {
> >>>>>>>>> +        Qcow2BitmapInfoList *bitmaps;
> >>>>>>>>> +        Error *local_err = NULL;
> >>>>>>>>> +
> >>>>>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
> >>>>>>>>
> >>>>>>>> Here you even had a good error message to print...
> >>>>>>>>
> >>>>>>>>>              *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> >>>>>>>>>                  .compat             = g_strdup("1.1"),
> >>>>>>>>>                  .lazy_refcounts     = s->compatible_features &
> >>>>>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> >>>>>>>>>                                        QCOW2_INCOMPAT_CORRUPT,
> >>>>>>>>>                  .has_corrupt        = true,
> >>>>>>>>>                  .refcount_bits      = s->refcount_bits,
> >>>>>>>>> +            .has_bitmaps        = !local_err,
> >>>>>>>>> +            .bitmaps            = bitmaps,
> >>>>>>>>>              };
> >>>>>>>>> +        /*
> >>>>>>>>> +         * If an error occurs in obtaining bitmaps, ignore
> >>>>>>>>> +         * it to show other QCOW2 specific information.
> >>>>>>>>> +         */
> >>>>>>>>> +        error_free(local_err);
> >>>>>>>>
> >>>>>>>> ...but you decided to discard it.
> >>>>>>>>
> >>>>>>>> How about you do this here:
> >>>>>>>>
> >>>>>>>>         warn_reportf_err(local_err, "Failed to load bitmap list: ");
> >>>>>>>
> >>>>>>> In case of JSON format, we can fail here.
> >>>>>>> It will happen in the current implementation of the new test #239.
> >>>>>>
> >>>>>> Why do you want to silently leave out the bitmap list for JSON output?
> >>>>>>
> >>>>>> Essentially, this is the same question I asked here:
> >>>>>>
> >>>>>>>> And then get rid of the code in block/qapi.c and the version 2 path?
> >>>>>>>>
> >>>>>>>> Actually, should this really only be a warning, or in fact an error?
> >>>>>>>> What's the case where we expect that loading the bitmap list can fail,
> >>>>>>>> but the management tool doesn't need to know that and we just want to
> >>>>>>>> log a message?
> >>>>>>
> >>>>>> I don't understand why it's okay not to print bitmaps without making it
> >>>>>> an error. Do you have a specific use case in mind for this behaviour?
> >>>>>>
> >>>>>
> >>>>>
> >>>>> We just can't print anything here, as this code is executed from qmp command.
> >>>>>
> >>>>> It was discussed, that we don't want to fail the whole query, if failed to
> >>>>> obtain bitmaps.
> >>>>
> >>>> It's obvious that you don't want to fail the query command, but I
> >>>> haven't found any explanation for _why_ you want this.
> >>>>
> >>>> The thing that is closest to an explanation is "it may be sad to fail
> >>>> get any information because of repeating some disk/qcow2 error", written
> >>>> by you in the v4 thread.
> >>>>
> >>>> I don't think "may be sad" is a good justification for non-standard
> >>>> behaviour. If we can't load the bitmaps, the image is broken. And if the
> >>>> image is broken, the rest of the information may be invalid, too.
> >>>
> >>> So, you mean that we go wrong way. And it was my idea. That is sad too.
> >>>
> >>> Than, seems like we should add errp to the function and to the full stack
> >>> down to qmp_query_block. And drop extra partial-success qapi logic.
> >>
> >> I'm not necessarily saying that it's the wrong way, though possibly it
> >> is wrong indeed.
> >>
> >> But ignoring some kind of failures is special, so what I was looking for
> >> were comments or documentation to explain the reason behind it at least.
> >> Now from the replies I got so far it looks to me that possibly the
> >> reasons are not only undocumented, but we might actually not have any.
> >>
> >>>>> So, to show, that there were error during bitmaps querying
> >>>>> we decided to use the following semantics:
> >>>>>
> >>>>> empty list (has_bitmaps=true) means that there no bitmaps, and there were
> >>>>> no errors during bitmaps quering (if it was)
> >>>>>
> >>>>> absence of the field (has_bitmaps=false) means that there _were_ errors during
> >>>>> bitmaps querying.
> >>>>
> >>>> ...or that you're running an old QEMU version. I'm not sure if making
> >>>> old QEMU versions and image errors look the same is a good move.
> >>>
> >>> No, for old versions we return empty list, to show that there were no errors.
> >>
> >> You mean old image format versions?
> > 
> > yes.
> > 
> > I'm talking about old QEMU versions
> >> that don't even know the 'bitmaps' field.
> > 
> > hmm. Yes, that's a problem, which we didn't considered.
> > 
> >>
> >> A QMP client would have to parse the schema to understand whether a
> >> missing 'bitmaps' field means that it's talking to an old QEMU (then
> >> 'bitmaps' doesn't exist in the schema), or that an error happened (then
> >> the field does exist in the schema).
> >>
> >> Looking at the schema is not impossible, so if we require this for a
> >> good reason, it's okay. But it's not trivial either. If we really want
> >> to go with this semantics, we should probably mention both the old and
> >> the new meaning in the QAPI documentation, with the recommendation that
> >> you parse the schema to determine the meaning of a missing 'bitmaps'
> >> field.
> > 
> > Hm, now I think that if we really face the case when we need partial success
> > of info querying, it should be additional optional parameter which enables it, like
> > skip-failed=true (default false) or something, which is more clear than parsing the
> > schema. And, which we can add later if needed.
> 
> Note also, that we already skip some errors, for example, block_crypto_get_specific_info_luks
> will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and
> bdrv_query_image_info skip these errors, in same manner as for formats which don't support
> bdrv_get_specific_info.
> 
> Looks like a good moment to fix this too, do you agree?

Yes, once we add an Error **errp to .bdrv_get_specific_info, returning
errors for failing qcrypto_block_get_info() calls as well (both in
crypto and qcow2) makes sense to me.

Or in fact, remove the errp parameter from qcrypto_block_get_info(). It
seems to never return errors.

Kevin

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

[Andrey Shinkevich]

On 29/01/2019 18:49, Kevin Wolf wrote:
> Am 29.01.2019 um 16:29 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.01.2019 17:35, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.01.2019 17:23, Kevin Wolf wrote:
>>>> Am 29.01.2019 um 15:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>> 29.01.2019 16:17, Kevin Wolf wrote:
>>>>>> Am 29.01.2019 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>>> 29.01.2019 15:38, Kevin Wolf wrote:
>>>>>>>> Am 29.01.2019 um 13:04 hat Andrey Shinkevich geschrieben:
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/block/qapi.c b/block/qapi.c
>>>>>>>>>>> index c66f949..0fde98c 100644
>>>>>>>>>>> --- a/block/qapi.c
>>>>>>>>>>> +++ b/block/qapi.c
>>>>>>>>>>> @@ -38,6 +38,7 @@
>>>>>>>>>>>       #include "qapi/qmp/qstring.h"
>>>>>>>>>>>       #include "sysemu/block-backend.h"
>>>>>>>>>>>       #include "qemu/cutils.h"
>>>>>>>>>>> +#include "qemu/error-report.h"
>>>>>>>>>>>       BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>>>>>>                                               BlockDriverState *bs, Error **errp)
>>>>>>>>>>> @@ -868,6 +869,11 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
>>>>>>>>>>>           if (info->has_format_specific) {
>>>>>>>>>>>               func_fprintf(f, "Format specific information:\n");
>>>>>>>>>>> +        if (info->format_specific &&
>>>>>>>>>>> +            info->format_specific->type == IMAGE_INFO_SPECIFIC_KIND_QCOW2 &&
>>>>>>>>>>> +            info->format_specific->u.qcow2.data->has_bitmaps == false) {
>>>>>>>>>>> +            warn_report("Failed to load bitmap list");
>>>>>>>>>>> +        }
>>>>>>>>>>>               bdrv_image_info_specific_dump(func_fprintf, f, info->format_specific);
>>>>>>>>>>>           }
>>>>>>>>>>>       }
>>>>>>>>>>
>>>>>>>>>> Is it really necessary to introduce qcow2 specific knowledge in
>>>>>>>>>> block/qapi.c (where it definitely doesn't belong), just to emit a
>>>>>>>>>> warning?
>>>>>>>>>>
>>>>>>>>>> Why can't you print the warning in qcow2_get_specific_info()?
>>>>>>>>>
>>>>>>>>> Dear Kevin,
>>>>>>>>> The reason behind the idea to move the warning to qapi is that we
>>>>>>>>> wouldn't like to print that in case of JSON format.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>>>> index 4897aba..07b99ee 100644
>>>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>>>> @@ -4386,8 +4386,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>>>>               *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>>>>                   .compat             = g_strdup("0.10"),
>>>>>>>>>>>                   .refcount_bits      = s->refcount_bits,
>>>>>>>>>>> +            .has_bitmaps        = true, /* To handle error check properly */
>>>>>>>>>>> +            .bitmaps            = NULL, /* Unsupported for version 2 */
>>>>>>>>>>
>>>>>>>>>> .has_bitmaps = false would be nicer if the format doesn't even support
>>>>>>>>>> bitmaps. You only need this here because you put the warning in the
>>>>>>>>>> wrong place.
>>>>>>>>>>
>>>>>>>>>>>               };
>>>>>>>>>>>           } else if (s->qcow_version == 3) {
>>>>>>>>>>> +        Qcow2BitmapInfoList *bitmaps;
>>>>>>>>>>> +        Error *local_err = NULL;
>>>>>>>>>>> +
>>>>>>>>>>> +        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
>>>>>>>>>>
>>>>>>>>>> Here you even had a good error message to print...
>>>>>>>>>>
>>>>>>>>>>>               *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>>>>>>>>>>>                   .compat             = g_strdup("1.1"),
>>>>>>>>>>>                   .lazy_refcounts     = s->compatible_features &
>>>>>>>>>>> @@ -4397,7 +4403,14 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>>>>>>>>>>>                                         QCOW2_INCOMPAT_CORRUPT,
>>>>>>>>>>>                   .has_corrupt        = true,
>>>>>>>>>>>                   .refcount_bits      = s->refcount_bits,
>>>>>>>>>>> +            .has_bitmaps        = !local_err,
>>>>>>>>>>> +            .bitmaps            = bitmaps,
>>>>>>>>>>>               };
>>>>>>>>>>> +        /*
>>>>>>>>>>> +         * If an error occurs in obtaining bitmaps, ignore
>>>>>>>>>>> +         * it to show other QCOW2 specific information.
>>>>>>>>>>> +         */
>>>>>>>>>>> +        error_free(local_err);
>>>>>>>>>>
>>>>>>>>>> ...but you decided to discard it.
>>>>>>>>>>
>>>>>>>>>> How about you do this here:
>>>>>>>>>>
>>>>>>>>>>          warn_reportf_err(local_err, "Failed to load bitmap list: ");
>>>>>>>>>
>>>>>>>>> In case of JSON format, we can fail here.
>>>>>>>>> It will happen in the current implementation of the new test #239.
>>>>>>>>
>>>>>>>> Why do you want to silently leave out the bitmap list for JSON output?
>>>>>>>>
>>>>>>>> Essentially, this is the same question I asked here:
>>>>>>>>
>>>>>>>>>> And then get rid of the code in block/qapi.c and the version 2 path?
>>>>>>>>>>
>>>>>>>>>> Actually, should this really only be a warning, or in fact an error?
>>>>>>>>>> What's the case where we expect that loading the bitmap list can fail,
>>>>>>>>>> but the management tool doesn't need to know that and we just want to
>>>>>>>>>> log a message?
>>>>>>>>
>>>>>>>> I don't understand why it's okay not to print bitmaps without making it
>>>>>>>> an error. Do you have a specific use case in mind for this behaviour?
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> We just can't print anything here, as this code is executed from qmp command.
>>>>>>>
>>>>>>> It was discussed, that we don't want to fail the whole query, if failed to
>>>>>>> obtain bitmaps.
>>>>>>
>>>>>> It's obvious that you don't want to fail the query command, but I
>>>>>> haven't found any explanation for _why_ you want this.
>>>>>>
>>>>>> The thing that is closest to an explanation is "it may be sad to fail
>>>>>> get any information because of repeating some disk/qcow2 error", written
>>>>>> by you in the v4 thread.
>>>>>>
>>>>>> I don't think "may be sad" is a good justification for non-standard
>>>>>> behaviour. If we can't load the bitmaps, the image is broken. And if the
>>>>>> image is broken, the rest of the information may be invalid, too.
>>>>>
>>>>> So, you mean that we go wrong way. And it was my idea. That is sad too.
>>>>>
>>>>> Than, seems like we should add errp to the function and to the full stack
>>>>> down to qmp_query_block. And drop extra partial-success qapi logic.
>>>>
>>>> I'm not necessarily saying that it's the wrong way, though possibly it
>>>> is wrong indeed.
>>>>
>>>> But ignoring some kind of failures is special, so what I was looking for
>>>> were comments or documentation to explain the reason behind it at least.
>>>> Now from the replies I got so far it looks to me that possibly the
>>>> reasons are not only undocumented, but we might actually not have any.
>>>>
>>>>>>> So, to show, that there were error during bitmaps querying
>>>>>>> we decided to use the following semantics:
>>>>>>>
>>>>>>> empty list (has_bitmaps=true) means that there no bitmaps, and there were
>>>>>>> no errors during bitmaps quering (if it was)
>>>>>>>
>>>>>>> absence of the field (has_bitmaps=false) means that there _were_ errors during
>>>>>>> bitmaps querying.
>>>>>>
>>>>>> ...or that you're running an old QEMU version. I'm not sure if making
>>>>>> old QEMU versions and image errors look the same is a good move.
>>>>>
>>>>> No, for old versions we return empty list, to show that there were no errors.
>>>>
>>>> You mean old image format versions?
>>>
>>> yes.
>>>
>>> I'm talking about old QEMU versions
>>>> that don't even know the 'bitmaps' field.
>>>
>>> hmm. Yes, that's a problem, which we didn't considered.
>>>
>>>>
>>>> A QMP client would have to parse the schema to understand whether a
>>>> missing 'bitmaps' field means that it's talking to an old QEMU (then
>>>> 'bitmaps' doesn't exist in the schema), or that an error happened (then
>>>> the field does exist in the schema).
>>>>
>>>> Looking at the schema is not impossible, so if we require this for a
>>>> good reason, it's okay. But it's not trivial either. If we really want
>>>> to go with this semantics, we should probably mention both the old and
>>>> the new meaning in the QAPI documentation, with the recommendation that
>>>> you parse the schema to determine the meaning of a missing 'bitmaps'
>>>> field.
>>>
>>> Hm, now I think that if we really face the case when we need partial success
>>> of info querying, it should be additional optional parameter which enables it, like
>>> skip-failed=true (default false) or something, which is more clear than parsing the
>>> schema. And, which we can add later if needed.
>>
>> Note also, that we already skip some errors, for example, block_crypto_get_specific_info_luks
>> will return NULL on errors (unlike qcow2_get_specific_info, which aborts), and
>> bdrv_query_image_info skip these errors, in same manner as for formats which don't support
>> bdrv_get_specific_info.
>>
>> Looks like a good moment to fix this too, do you agree?
> 
> Yes, once we add an Error **errp to .bdrv_get_specific_info, returning
> errors for failing qcrypto_block_get_info() calls as well (both in
> crypto and qcow2) makes sense to me.
> 
> Or in fact, remove the errp parameter from qcrypto_block_get_info(). It
> seems to never return errors.
> 
> Kevin
> 

Dear Kevin,

Thank you very much for your collaboration.
Your suggestion makes the code change even simpler.
I have modified the patches.
Please review a new version #10.

-- 
With the best regards,
Andrey Shinkevich

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

[Andrey Shinkevich]

On 28/01/2019 23:43, Eric Blake wrote:
> On 1/28/19 2:01 PM, 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:
>>
> 
>>
>> As the print of the qcow2 specific information expanded by adding
>> the bitmap parameters to the 'qemu-img info' command output,
>> it requires amendment of the output benchmark in the following
>> tests: 060, 065, 082, 198, and 206.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
> 
>>   
>> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
>> +{
>> +    Qcow2BitmapInfoFlagsList *list = NULL;
>> +    Qcow2BitmapInfoFlagsList **plist = &list;
>> +
>> +    if (flags & BME_FLAG_IN_USE) {
>> +        Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
>> +        entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
>> +        *plist = entry;
>> +        plist = &entry->next;
> 
> This line...
> 
>> +    }
>> +    if (flags & BME_FLAG_AUTO) {
>> +        Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
>> +        entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
>> +        *plist = entry;
>> +    }
> 
> ...is omitted here. Harmless for now, but may cause grief if a later
> flag is added and we forget to add it in. On the other hand, I don't
> know if a static analyzer might warn about a dead assignment, so
> breaking the symmetry between the two is okay if that is the justification.
> 
> Also, thinking about future flag additions, would it make any sense to
> write this code in a for loop?  Something like (untested):
> 
>      static const struct Map {
>          int bme;
>          int info;
>      } map[] = {
>          { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
>          { BME_FLAG_AUTO,   QCOW2_BITMAP_INFO_FLAGS_AUTO },
>      };
> 
>      for (i = 0; i < ARRAY_LENGTH(map); i++) {
>          if (flags & map[i].bme) {
>              ...; entry->value = map[i].info;
>      }
> 
> where adding a new bit is now a one-liner change to the definition of
> 'map' rather than a 6-line addition of a new conditional.
> 
> 
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: flags of the bitmap
>> +#
>> +# @unknown-flags: unspecified flags if detected
> 
> Maybe:
> 
> @flags: recognized flags of the bitmap
> 
> @unknown-flags: any remaining flags not recognized by this qemu version
> 
> 
>> +++ b/tests/qemu-iotests/060.out
>> @@ -18,6 +18,7 @@ cluster_size: 65536
>>   Format specific information:
>>       compat: 1.1
>>       lazy refcounts: false
>> +    bitmaps:
> 
> Hmm. I'm wondering if the human-readable output of a QAPI type with an
> optional array should output "<none>" or something similar for a
> 0-element array, to make it obvious to the human reading the output that
> there are no bitmaps.  That's not necessarily a problem in your patch;
> and may have even bigger effects to other iotests, so it should be done
> as a separate patch if we want it.  But even in your patch, if we did
> that,...
> 
>>       refcount bits: 16
>>       corrupt: true
>>   can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write
>> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
>> index 8bac383..86406cb 100755
>> --- a/tests/qemu-iotests/065
>> +++ b/tests/qemu-iotests/065
>> @@ -88,23 +88,23 @@ class TestQMP(TestImageInfoSpecific):
>>   class TestQCow2(TestQemuImgInfo):
>>       '''Testing a qcow2 version 2 image'''
>>       img_options = 'compat=0.10'
>> -    json_compare = { 'compat': '0.10', 'refcount-bits': 16 }
>> -    human_compare = [ 'compat: 0.10', 'refcount bits: 16' ]
>> +    json_compare = { 'compat': '0.10', 'bitmaps': [], 'refcount-bits': 16 }
>> +    human_compare = [ 'compat: 0.10', 'bitmaps:', 'refcount bits: 16' ]
> 
> ...the human_compare dict would have to account for whatever string we
> output for an empty JSON array.
> 
> I'm finding the functionality useful, though, so unless there are strong
> opinions presented on making further tweaks, I'm also fine giving this
> version as-is:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Dear Eric,

Thank you very much for your foresight and considerate approach.
I have made changes in the series and am about to send a new version #10.
Please review that.
I appreciate your collaboration.

-- 
With the best regards,
Andrey Shinkevich