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

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

[Andrey Shinkevich]

v11:
An assertion was added to the get_bitmap_info_flags() to check the
completed mapping of all the reserved bitmap BME_ flags.
The heading comment of get_bitmap_info_flags() was changed to
describe the function design properly.
In qcow2_get_specific_info(), two function calls to g_free were
replaced with one call to qapi_free_ImageInfoSpecific() that does
all the cleaning work.

v10:
The 'struct Error' parameter was added to the function prototype
bdrv_query_image_info().
The code refactoring of the function get_bitmap_info_flags().
The comments to the structures ImageInfoSpecificQCow2 and
Qcow2BitmapInfo in the file qapi/block-core.json were corrected.
The changes in the *.out files of the tests 060, 065 082, 198
and 206 were discarded. The new test 239 was enriched by adding human
readable format output and by checking the output with bitmap extra
parameters, such as  non-persistent and disabled.
The version #10 was discussed in email thread with the message ID:
<1548870690-647481-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".
The version #9 was discussed with the message ID:
<1548705688-1027522-1-git-send-email-andrey.shinkevich@virtuozzo.com>

v8:
The output benchmark files for the qemu-iotests, namely 060, 065 082, 198
and 206, were modified to show the bitmap extension for the qemu specific
information. A new test file 239 was added to the test set that checks the
output for the fields of the bitmap section.
The backward compatibility of the output for images of the version 2
of qcow2 was added.
The version #8 was discussed in email thread with the message ID:
<1548700805-1016533-1-git-send-email-andrey.shinkevich@virtuozzo.com>

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.
The version #7 was discussed in email thread with the message ID:
<1544698788-52893-1-git-send-email-andrey.shinkevich@virtuozzo.com>

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

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

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

v3:
'[PATCH v3] qemu-img info lists bitmap directory entries'.
Now, qcow2_get_bitmap_info_list() is invoked under the condition of QCOW
version #3 to avoid memory leaks in case of QCOW version #2.
Furthermore, qcow2_get_bitmap_info_list() checks the number of existing bitmaps.
So, if no bitmap exists, no bitmap error message is printed in the output.
The data type of the bitmap 'granularity' parameter was left as 'uint32'
because bitmap_list_load() returns error if granularity_bits is grater than 31.

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

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

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

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

-- 
1.8.3.1

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

[Andrey Shinkevich]

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

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

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

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

[Andrey Shinkevich]

In the 'Format specific information' section of the 'qemu-img info'
command output, the supplemental information about existing QCOW2
bitmaps will be shown, such as a bitmap name, flags and granularity:

image: /vz/vmprivate/VM1/harddisk.hdd
file format: qcow2
virtual size: 64G (68719476736 bytes)
disk size: 3.0M
cluster_size: 1048576
Format specific information:
    compat: 1.1
    lazy refcounts: true
    bitmaps:
        [0]:
            flags:
                [0]: in-use
                [1]: auto
            name: back-up1
            unknown flags: 4
            granularity: 65536
        [1]:
            flags:
                [0]: in-use
                [1]: auto
            name: back-up2
            unknown flags: 8
            granularity: 65536
    refcount bits: 16
    corrupt: false

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/qcow2-bitmap.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        | 11 +++++++-
 block/qcow2.h        |  2 ++
 qapi/block-core.json | 40 ++++++++++++++++++++++++++-
 4 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b946301..516994e 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1006,6 +1006,84 @@ fail:
     return false;
 }
 
+
+static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
+{
+    Qcow2BitmapInfoFlagsList *list = NULL;
+    Qcow2BitmapInfoFlagsList **plist = &list;
+    int i;
+
+    static const struct {
+        int bme;  /* Bitmap directory entry flags */
+        int info; /* The flags to report to the user */
+    } map[] = {
+        { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
+        { BME_FLAG_AUTO,   QCOW2_BITMAP_INFO_FLAGS_AUTO },
+    };
+
+    int map_size = sizeof(map) / sizeof(map[0]);
+
+    for (i = 0; i < map_size; ++i) {
+        if (flags & map[i].bme) {
+            Qcow2BitmapInfoFlagsList *entry =
+                g_new0(Qcow2BitmapInfoFlagsList, 1);
+            entry->value = map[i].info;
+            *plist = entry;
+            plist = &entry->next;
+            flags &= ~map[i].bme;
+        }
+    }
+    /* Check if the BME_* mapping above is complete */
+    assert(!flags);
+
+    return list;
+}
+
+/*
+ * qcow2_get_bitmap_info_list()
+ * Returns a list of QCOW2 bitmap details.
+ * In case of no bitmaps, the function returns NULL and
+ * the @errp parameter is not set.
+ * When bitmap information can not be obtained, the function returns
+ * NULL and the @errp parameter is set.
+ */
+Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
+                                                Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+    Qcow2BitmapInfoList *list = NULL;
+    Qcow2BitmapInfoList **plist = &list;
+
+    if (s->nb_bitmaps == 0) {
+        return NULL;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, errp);
+    if (bm_list == NULL) {
+        return NULL;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
+        Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
+        info->granularity = 1U << bm->granularity_bits;
+        info->name = g_strdup(bm->name);
+        info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
+        info->unknown_flags = bm->flags & BME_RESERVED_FLAGS;
+        info->has_unknown_flags = !!info->unknown_flags;
+        obj->value = info;
+        *plist = obj;
+        plist = &obj->next;
+    }
+
+    bitmap_list_free(bm_list);
+
+    return list;
+}
+
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                  Error **errp)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 27e5a2c..a5607f1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4386,7 +4386,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
     spec_info = g_new(ImageInfoSpecific, 1);
     *spec_info = (ImageInfoSpecific){
         .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
-        .u.qcow2.data = g_new(ImageInfoSpecificQCow2, 1),
+        .u.qcow2.data = g_new0(ImageInfoSpecificQCow2, 1),
     };
     if (s->qcow_version == 2) {
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
@@ -4394,6 +4394,13 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
             .refcount_bits      = s->refcount_bits,
         };
     } else if (s->qcow_version == 3) {
+        Qcow2BitmapInfoList *bitmaps;
+        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            qapi_free_ImageInfoSpecific(spec_info);
+            return NULL;
+        }
         *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
             .compat             = g_strdup("1.1"),
             .lazy_refcounts     = s->compatible_features &
@@ -4403,6 +4410,8 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
                                   QCOW2_INCOMPAT_CORRUPT,
             .has_corrupt        = true,
             .refcount_bits      = s->refcount_bits,
+            .has_bitmaps        = !!bitmaps,
+            .bitmaps            = bitmaps,
         };
     } else {
         /* if this assertion fails, this probably means a new version was
diff --git a/block/qcow2.h b/block/qcow2.h
index 438a1de..13e8964 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -684,6 +684,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
+                                                Error **errp);
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 91685be..271e0df 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -69,6 +69,8 @@
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @bitmaps: A list of qcow2 bitmap details (since 4.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +79,8 @@
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['Qcow2BitmapInfo']
   } }
 
 ##
@@ -454,6 +457,41 @@
            'status': 'DirtyBitmapStatus'} }
 
 ##
+# @Qcow2BitmapInfoFlags:
+#
+# An enumeration of flags that a bitmap can report to the user.
+#
+# @in-use: The bitmap was not saved correctly and may be inconsistent.
+#
+# @auto: The bitmap must reflect all changes of the virtual disk by any
+#        application that would write to this qcow2 file.
+#
+# Since: 4.0
+##
+{ 'enum': 'Qcow2BitmapInfoFlags',
+  'data': ['in-use', 'auto'] }
+
+##
+# @Qcow2BitmapInfo:
+#
+# Qcow2 bitmap information.
+#
+# @name: the name of the bitmap
+#
+# @granularity: granularity of the bitmap in bytes
+#
+# @flags: recognized flags of the bitmap
+#
+# @unknown-flags: any remaining flags not recognized by the current qemu version
+#
+# Since: 4.0
+##
+{ 'struct': 'Qcow2BitmapInfo',
+  'data': {'name': 'str', 'granularity': 'uint32',
+           'flags': ['Qcow2BitmapInfoFlags'],
+           '*unknown-flags': 'uint32' } }
+
+##
 # @BlockLatencyHistogramInfo:
 #
 # Block latency histogram.
-- 
1.8.3.1

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

[Andrey Shinkevich]

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

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

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

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

[Vladimir Sementsov-Ogievskiy]

31.01.2019 16:46, Andrey Shinkevich wrote:
> Inform a user in case qcow2_get_specific_info fails to obtain
> QCOW2 image specific information. This patch is preliminary to
> the print of bitmap information in the 'qemu-img info' output.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---


[...]

> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -594,13 +594,13 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
>   }
>   
>   static ImageInfoSpecific *
> -block_crypto_get_specific_info_luks(BlockDriverState *bs)
> +block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
>   {
>       BlockCrypto *crypto = bs->opaque;
>       ImageInfoSpecific *spec_info;
>       QCryptoBlockInfo *info;
>   
> -    info = qcrypto_block_get_info(crypto->block, NULL);
> +    info = qcrypto_block_get_info(crypto->block, errp);
>       if (!info) {
>           return NULL;
>       }

more context:

       if (info->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
           qapi_free_QCryptoBlockInfo(info);
           return NULL;
       }

for a fast look, I think it should be assertion, not if, Daniel, am I right?
Also, I think we don't have block/crypto.c in Cryptography section of MAINTAINERS
by mistake, so you were not CC'ed.


> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949..f53f100 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -282,7 +282,12 @@ void bdrv_query_image_info(BlockDriverState *bs,
>           info->dirty_flag = bdi.is_dirty;
>           info->has_dirty_flag = true;
>       }
> -    info->format_specific     = bdrv_get_specific_info(bs);
> +    info->format_specific     = bdrv_get_specific_info(bs, &err);

while being here, let's drop these extra whitespaces

> +    if (err) {
> +        error_propagate(errp, err);
> +        qapi_free_ImageInfo(info);
> +        goto out;
> +    }
>       info->has_format_specific = info->format_specific != NULL;
>   
>       backing_filename = bs->backing_file;


So, I'm unsure about crypto, but it's safe to keep it as is, so for this patch:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

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

[Daniel P. Berrangé]

On Fri, Feb 01, 2019 at 02:42:10PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 31.01.2019 16:46, Andrey Shinkevich wrote:
> > Inform a user in case qcow2_get_specific_info fails to obtain
> > QCOW2 image specific information. This patch is preliminary to
> > the print of bitmap information in the 'qemu-img info' output.
> > 
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> 
> 
> [...]
> 
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -594,13 +594,13 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
> >   }
> >   
> >   static ImageInfoSpecific *
> > -block_crypto_get_specific_info_luks(BlockDriverState *bs)
> > +block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
> >   {
> >       BlockCrypto *crypto = bs->opaque;
> >       ImageInfoSpecific *spec_info;
> >       QCryptoBlockInfo *info;
> >   
> > -    info = qcrypto_block_get_info(crypto->block, NULL);
> > +    info = qcrypto_block_get_info(crypto->block, errp);
> >       if (!info) {
> >           return NULL;
> >       }
> 
> more context:
> 
>        if (info->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
>            qapi_free_QCryptoBlockInfo(info);
>            return NULL;
>        }
> 
> for a fast look, I think it should be assertion, not if, Daniel, am I right?

Sure, it could be an assertion. In practice it should never trigger
eitherway.

> Also, I think we don't have block/crypto.c in Cryptography section of MAINTAINERS
> by mistake, so you were not CC'ed.

I tend to let the block maintainers merge patches under block/,
since that's largely block layer integration. The guts of the
block crypto stuff is under crypto/


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Vladimir Sementsov-Ogievskiy]

31.01.2019 16:46, 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:
> 
> image: /vz/vmprivate/VM1/harddisk.hdd
> file format: qcow2
> virtual size: 64G (68719476736 bytes)
> disk size: 3.0M
> cluster_size: 1048576
> Format specific information:
>      compat: 1.1
>      lazy refcounts: true
>      bitmaps:
>          [0]:
>              flags:
>                  [0]: in-use
>                  [1]: auto
>              name: back-up1
>              unknown flags: 4
>              granularity: 65536
>          [1]:
>              flags:
>                  [0]: in-use
>                  [1]: auto
>              name: back-up2
>              unknown flags: 8
>              granularity: 65536
>      refcount bits: 16
>      corrupt: false
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

31.01.2019 16:46, Andrey Shinkevich wrote:
> A new test file 239 added to the qemu-iotests set. It checks
> the output format of 'qemu-img info' for bitmaps extension of
> qcow2 specific information.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/239     |  74 ++++++++++++++++++++++++++
>   tests/qemu-iotests/239.out | 130 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 205 insertions(+)
>   create mode 100755 tests/qemu-iotests/239
>   create mode 100644 tests/qemu-iotests/239.out
> 
> diff --git a/tests/qemu-iotests/239 b/tests/qemu-iotests/239
> new file mode 100755
> index 0000000..bee7943
> --- /dev/null
> +++ b/tests/qemu-iotests/239
> @@ -0,0 +1,74 @@
> +#!/usr/bin/env python
> +#
> +# Test for qcow2 bitmap printed information
> +#
> +# Copyright (c) 2018 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import iotests
> +import json
> +from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
> +                    file_path, log
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +disk = file_path('disk')
> +chunk = 256
> +
> +def print_bitmap():
> +    log('qemu-img info dump:\n')
> +    iotests.img_info_log(disk, extra_args=['--force-share'])
> +    result = json.loads(qemu_img_pipe('info', '--force-share',
> +                                      '--output=json', disk))
> +    if 'bitmaps' in result['format-specific']['data']:
> +        bitmaps = result['format-specific']['data']['bitmaps']
> +        log('The same bitmaps in JSON format:')
> +        log(bitmaps, indent=2)
> +    else:
> +        log('No bitmap in JSON format output')
> +
> +def add_bitmap(bitmap_number, persistent, disabled):
> +    granularity = 2**(13 + bitmap_number)
> +    bitmap_name = 'bitmap-' + str(bitmap_number-1)
> +    vm = iotests.VM().add_drive(disk)
> +    vm.launch()
> +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
> +               granularity=granularity, persistent=persistent,
> +               disabled=disabled)
> +    vm.shutdown()
> +
> +def write_to_disk(offset, size):
> +    write = 'write {} {}K'.format(offset, size)

I think, you meant {}K {}K. On the other hand, if you want to work with 256K chunks,
I think, better is to define chunk as
chunk = 256 * 1024

> +    qemu_io('-f', iotests.imgfmt, '-c', write, disk)

Hmm, interesting, it's a common preactice to give -f iotests.imgfmt to qemu_io(),
but in reality it's not needed, as qemu_io() automatically set this flag, this
parameter will be passed twice. So, it would be good to fix it in all iotests,
handle '-f' properly in qemu_io(), and I think, in qemu-io utility too, forbid
multiple -f option.

So, here let's use
qemu_io('-c', write, disk)

> +    log('Write ' + str(size) + 'K to disk at offset ' + str(hex(offset)))

hmm, you just do qemu-io output by hand. better log() what qemu_io() returns
with appropriate filters, look in iotest 149

> +
> +
> +qemu_img_create('-f', iotests.imgfmt, disk, '1M')
> +
> +for num in range(1, 4):

good to log('Test {}'.format(num)) here

> +    disabled = False
> +    if num == 2:
> +        disabled = True
> +    add_bitmap(num, bool(num-1), disabled)

more clear: num > 1

> +    write_to_disk((num-1)*chunk, chunk)

Hm, actually this don't change test output, as we don't see dirty bits through
new API. However, it make sense anyway, to be closer to real-life.

> +    print_bitmap()
> +    log('')
> +
> +vm = iotests.VM().add_drive(disk)
> +vm.launch()
> +log('Checking \"in-use\" flag...')

don't back-slash double quotes, it's not needed, as you are inside single quotes.

> +print_bitmap()

I'd prefere to use '--force-share' only for this case, when it's really needed.

> +vm.shutdown()
> diff --git a/tests/qemu-iotests/239.out b/tests/qemu-iotests/239.out



-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 1/31/19 7:46 AM, Andrey Shinkevich wrote:
> In the 'Format specific information' section of the 'qemu-img info'
> command output, the supplemental information about existing QCOW2
> bitmaps will be shown, such as a bitmap name, flags and granularity:
> 

> +
> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
> +{
> +    Qcow2BitmapInfoFlagsList *list = NULL;
> +    Qcow2BitmapInfoFlagsList **plist = &list;
> +    int i;
> +
> +    static const struct {
> +        int bme;  /* Bitmap directory entry flags */
> +        int info; /* The flags to report to the user */
> +    } map[] = {
> +        { BME_FLAG_IN_USE, QCOW2_BITMAP_INFO_FLAGS_IN_USE },
> +        { BME_FLAG_AUTO,   QCOW2_BITMAP_INFO_FLAGS_AUTO },
> +    };
> +
> +    int map_size = sizeof(map) / sizeof(map[0]);

We have the ARRAY_SIZE() macro for this.

> +
> +    for (i = 0; i < map_size; ++i) {
> +        if (flags & map[i].bme) {
> +            Qcow2BitmapInfoFlagsList *entry =
> +                g_new0(Qcow2BitmapInfoFlagsList, 1);
> +            entry->value = map[i].info;
> +            *plist = entry;
> +            plist = &entry->next;
> +            flags &= ~map[i].bme;
> +        }
> +    }
> +    /* Check if the BME_* mapping above is complete */
> +    assert(!flags);
> +

> +        info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
> +        info->unknown_flags = bm->flags & BME_RESERVED_FLAGS;
> +        info->has_unknown_flags = !!info->unknown_flags;

Glad you liked my idea for better sanity checking.

If that's the only change needed, I don't mind making it as part of
staging for a pull request (of course, review comments on 3/3 may still
result in a v12 for other reasons).

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 v11 1/3] bdrv_query_image_info Error parameter added

[Kevin Wolf]

Am 31.01.2019 um 14:46 hat Andrey Shinkevich geschrieben:
> Inform a user in case qcow2_get_specific_info fails to obtain
> QCOW2 image specific information. This patch is preliminary to
> the print of bitmap information in the 'qemu-img info' output.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/block/crypto.c b/block/crypto.c
> index f0a5f6b..4ede833 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -594,13 +594,13 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
>  }
>  
>  static ImageInfoSpecific *
> -block_crypto_get_specific_info_luks(BlockDriverState *bs)
> +block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
>  {
>      BlockCrypto *crypto = bs->opaque;
>      ImageInfoSpecific *spec_info;
>      QCryptoBlockInfo *info;
>  
> -    info = qcrypto_block_get_info(crypto->block, NULL);
> +    info = qcrypto_block_get_info(crypto->block, errp);
>      if (!info) {
>          return NULL;
>      }

In v9 of the series I suggested to have another patch to remove errp
from qcrypto_block_get_info() because it never returns errors anyway.

Don't you think that would be better? But it's your decision, I'm fine
with either way.

> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949..f53f100 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -282,7 +282,12 @@ void bdrv_query_image_info(BlockDriverState *bs,
>          info->dirty_flag = bdi.is_dirty;
>          info->has_dirty_flag = true;
>      }
> -    info->format_specific     = bdrv_get_specific_info(bs);
> +    info->format_specific     = bdrv_get_specific_info(bs, &err);

The spacing looks odd now. I'd either keep the assignment for
info->has_format_specific above the if block so that both are aligned to
the same column, or remove the additional spaces.

With that fixed:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 31.01.2019 um 14:46 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>

239 is already taken, so please rename this one. I usually search the
mailing list for "239.out" etc. until I find a number that nobody has
used even in a patch series yet.

Kevin

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

[Kevin Wolf]

Am 31.01.2019 um 14:46 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
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

I haven't reviewed the patch in detail, but I suggest that you change
the subject line to make clear this is a qcow2 patch, not a qemu-img
one. Maybe:

    "qcow2: Add list of bitmaps to ImageInfoSpecificQCow2"

Kevin

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

[Eric Blake]

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

On 1/31/19 7:46 AM, Andrey Shinkevich wrote:
> A new test file 239 added to the qemu-iotests set. It checks
> the output format of 'qemu-img info' for bitmaps extension of
> qcow2 specific information.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  tests/qemu-iotests/239     |  74 ++++++++++++++++++++++++++
>  tests/qemu-iotests/239.out | 130 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 205 insertions(+)
>  create mode 100755 tests/qemu-iotests/239
>  create mode 100644 tests/qemu-iotests/239.out

Kevin just sent a pull request with 239 consumed for something else (a
test for dmg; also consumed 240); I don't mind renumbering the test as
242 or later as part of staging if we are happy with this version of the
test.  But I also see that Vladimir had some suggestions, so I wouldn't
be surprised if you want to send a v12.

> 
> diff --git a/tests/qemu-iotests/239 b/tests/qemu-iotests/239
> new file mode 100755
> index 0000000..bee7943
> --- /dev/null
> +++ b/tests/qemu-iotests/239
> @@ -0,0 +1,74 @@
> +#!/usr/bin/env python
> +#
> +# Test for qcow2 bitmap printed information
> +#
> +# Copyright (c) 2018 Virtuozzo International GmbH

Want to claim 2019?


> +def add_bitmap(bitmap_number, persistent, disabled):
> +    granularity = 2**(13 + bitmap_number)
> +    bitmap_name = 'bitmap-' + str(bitmap_number-1)
> +    vm = iotests.VM().add_drive(disk)
> +    vm.launch()
> +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
> +               granularity=granularity, persistent=persistent,
> +               disabled=disabled)
> +    vm.shutdown()
> +
> +def write_to_disk(offset, size):
> +    write = 'write {} {}K'.format(offset, size)
> +    qemu_io('-f', iotests.imgfmt, '-c', write, disk)
> +    log('Write ' + str(size) + 'K to disk at offset ' + str(hex(offset)))
> +
> +
> +qemu_img_create('-f', iotests.imgfmt, disk, '1M')
> +
> +for num in range(1, 4):
> +    disabled = False
> +    if num == 2:
> +        disabled = True
> +    add_bitmap(num, bool(num-1), disabled)

I don't know the pythonic way of coercing a bool value out of a check
for whether another value is a particular integer; but I see that you
did it in two different styles.  I don't know if this works, but it
would at least look more consistent:

add_bitmap(num, bool(num == 1), bool(num == 2))

> +    write_to_disk((num-1)*chunk, chunk)

Does PEP8 want spaces around those operators?

> +    print_bitmap()
> +    log('')
> +
> +vm = iotests.VM().add_drive(disk)
> +vm.launch()
> +log('Checking \"in-use\" flag...')
> +print_bitmap()
> +vm.shutdown()

The test output looks like reasonable coverage.
Tested-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 v11 3/3] qemu-img info: bitmaps extension new test 239

[Kevin Wolf]

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

Am 01.02.2019 um 18:23 hat Eric Blake geschrieben:
> On 1/31/19 7:46 AM, Andrey Shinkevich wrote:
> > A new test file 239 added to the qemu-iotests set. It checks
> > the output format of 'qemu-img info' for bitmaps extension of
> > qcow2 specific information.
> > 
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >  tests/qemu-iotests/239     |  74 ++++++++++++++++++++++++++
> >  tests/qemu-iotests/239.out | 130 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 205 insertions(+)
> >  create mode 100755 tests/qemu-iotests/239
> >  create mode 100644 tests/qemu-iotests/239.out
> 
> Kevin just sent a pull request with 239 consumed for something else (a
> test for dmg; also consumed 240); I don't mind renumbering the test as
> 242 or later as part of staging if we are happy with this version of the
> test.  But I also see that Vladimir had some suggestions, so I wouldn't
> be surprised if you want to send a v12.
> 
> > 
> > diff --git a/tests/qemu-iotests/239 b/tests/qemu-iotests/239
> > new file mode 100755
> > index 0000000..bee7943
> > --- /dev/null
> > +++ b/tests/qemu-iotests/239
> > @@ -0,0 +1,74 @@
> > +#!/usr/bin/env python
> > +#
> > +# Test for qcow2 bitmap printed information
> > +#
> > +# Copyright (c) 2018 Virtuozzo International GmbH
> 
> Want to claim 2019?
> 
> 
> > +def add_bitmap(bitmap_number, persistent, disabled):
> > +    granularity = 2**(13 + bitmap_number)
> > +    bitmap_name = 'bitmap-' + str(bitmap_number-1)
> > +    vm = iotests.VM().add_drive(disk)
> > +    vm.launch()
> > +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap_name,
> > +               granularity=granularity, persistent=persistent,
> > +               disabled=disabled)
> > +    vm.shutdown()
> > +
> > +def write_to_disk(offset, size):
> > +    write = 'write {} {}K'.format(offset, size)
> > +    qemu_io('-f', iotests.imgfmt, '-c', write, disk)
> > +    log('Write ' + str(size) + 'K to disk at offset ' + str(hex(offset)))
> > +
> > +
> > +qemu_img_create('-f', iotests.imgfmt, disk, '1M')
> > +
> > +for num in range(1, 4):
> > +    disabled = False
> > +    if num == 2:
> > +        disabled = True
> > +    add_bitmap(num, bool(num-1), disabled)
> 
> I don't know the pythonic way of coercing a bool value out of a check
> for whether another value is a particular integer; but I see that you
> did it in two different styles.  I don't know if this works, but it
> would at least look more consistent:
> 
> add_bitmap(num, bool(num == 1), bool(num == 2))

Hm...

    >>> num = 0
    >>> type(num == 2)
    <class 'bool'>

Why all the bool() calls when the result of == is already a bool?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Markus Armbruster]

Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:

> In the 'Format specific information' section of the 'qemu-img info'
> command output, the supplemental information about existing QCOW2
> bitmaps will be shown, such as a bitmap name, flags and granularity:
>
> image: /vz/vmprivate/VM1/harddisk.hdd
> file format: qcow2
> virtual size: 64G (68719476736 bytes)
> disk size: 3.0M
> cluster_size: 1048576
> Format specific information:
>     compat: 1.1
>     lazy refcounts: true
>     bitmaps:
>         [0]:
>             flags:
>                 [0]: in-use
>                 [1]: auto
>             name: back-up1
>             unknown flags: 4
>             granularity: 65536
>         [1]:
>             flags:
>                 [0]: in-use
>                 [1]: auto
>             name: back-up2
>             unknown flags: 8
>             granularity: 65536
>     refcount bits: 16
>     corrupt: false
>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 91685be..271e0df 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -69,6 +69,8 @@
>  # @encrypt: details about encryption parameters; only set if image
>  #           is encrypted (since 2.10)
>  #
> +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
> +#
>  # Since: 1.7
>  ##
>  { 'struct': 'ImageInfoSpecificQCow2',
> @@ -77,7 +79,8 @@
>        '*lazy-refcounts': 'bool',
>        '*corrupt': 'bool',
>        'refcount-bits': 'int',
> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> +      '*bitmaps': ['Qcow2BitmapInfo']
>    } }
>  
>  ##
> @@ -454,6 +457,41 @@
>             'status': 'DirtyBitmapStatus'} }
>  
>  ##
> +# @Qcow2BitmapInfoFlags:
> +#
> +# An enumeration of flags that a bitmap can report to the user.
> +#
> +# @in-use: The bitmap was not saved correctly and may be inconsistent.

I doubt the casual reader could guess the meaning from the name.  What
about @dirty?

> +#
> +# @auto: The bitmap must reflect all changes of the virtual disk by any
> +#        application that would write to this qcow2 file.
> +#
> +# Since: 4.0
> +##
> +{ 'enum': 'Qcow2BitmapInfoFlags',
> +  'data': ['in-use', 'auto'] }
> +
> +##
> +# @Qcow2BitmapInfo:
> +#
> +# Qcow2 bitmap information.
> +#
> +# @name: the name of the bitmap
> +#
> +# @granularity: granularity of the bitmap in bytes
> +#
> +# @flags: recognized flags of the bitmap
> +#
> +# @unknown-flags: any remaining flags not recognized by the current qemu version

Intended use cases for @unknown-flags?

> +#
> +# Since: 4.0
> +##
> +{ 'struct': 'Qcow2BitmapInfo',
> +  'data': {'name': 'str', 'granularity': 'uint32',
> +           'flags': ['Qcow2BitmapInfoFlags'],
> +           '*unknown-flags': 'uint32' } }
> +
> +##
>  # @BlockLatencyHistogramInfo:
>  #
>  # Block latency histogram.

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

[Eric Blake]

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

On 2/1/19 12:39 PM, Markus Armbruster wrote:
> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> 

>>  
>>  ##
>> +# @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.
> 
> I doubt the casual reader could guess the meaning from the name.  What
> about @dirty?

I like it.  The existing name was chosen to match
docs/interop/qcow2.txt, which uses in_use, but I don't see a problem in
making the UI nicer than the specs (and/or rewording the specs, as the
field name doesn't matter there, only the layout).


>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
> 
> Intended use cases for @unknown-flags?

The qcow2 spec defines bit 2 extra_data_compatible; and also leaves the
door open for future extensions that may define other bits. If a new
version of qemu (or some non-qemu qcow2 creation app) creates an image
with additional feature bits set, THIS version of qemu doesn't know what
name to give those bits, but can still inform the user that those bits
are set via this field. It will be omitted for all images created by
this version of qemu.

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 2/1/19 12:39 PM, Markus Armbruster wrote:
>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
>> 
>
>>>  
>>>  ##
>>> +# @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.
>> 
>> I doubt the casual reader could guess the meaning from the name.  What
>> about @dirty?
>
> I like it.  The existing name was chosen to match
> docs/interop/qcow2.txt, which uses in_use, but I don't see a problem in
> making the UI nicer than the specs (and/or rewording the specs, as the
> field name doesn't matter there, only the layout).
>
>
>>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
>> 
>> Intended use cases for @unknown-flags?
>
> The qcow2 spec defines bit 2 extra_data_compatible; and also leaves the
> door open for future extensions that may define other bits. If a new
> version of qemu (or some non-qemu qcow2 creation app) creates an image
> with additional feature bits set, THIS version of qemu doesn't know what
> name to give those bits, but can still inform the user that those bits
> are set via this field. It will be omitted for all images created by
> this version of qemu.

What would QMP clients do with this information?

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

[Eric Blake]

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

On 2/1/19 1:23 PM, Markus Armbruster wrote:

>>>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
>>>
>>> Intended use cases for @unknown-flags?
>>
>> The qcow2 spec defines bit 2 extra_data_compatible; and also leaves the
>> door open for future extensions that may define other bits. If a new
>> version of qemu (or some non-qemu qcow2 creation app) creates an image
>> with additional feature bits set, THIS version of qemu doesn't know what
>> name to give those bits, but can still inform the user that those bits
>> are set via this field. It will be omitted for all images created by
>> this version of qemu.
> 
> What would QMP clients do with this information?

'qemu-img info' is the intended QMP client; and it will print the
unknown-flags along with everything else. In other words, we're trying
to make qemu-img become useful for inspecting as much useful information
as possible from an image with unknown origins.  Knowing that an image
uses bitmaps with flags unknown to THIS version of qemu is a good
indication that you should be careful about using/altering the image
without first upgrading qemu, as it may destroy data that some other
product desires to utilize.

-- 
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 v11 0/3] qemu-img info lists bitmap directory entries

[no-reply]

Patchew URL: https://patchew.org/QEMU/1548942405-760115-1-git-send-email-andrey.shinkevich@virtuozzo.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===


Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/tmp/qemu-test/install --python=/usr/bin/python3 --cross-prefix=x86_64-w64-mingw32- --enable-trace-backends=simple --enable-gnutls --enable-nettle --enable-curl --enable-vnc --enable-bzip2 --enable-guest-agent --with-sdlabi=2.0
ERROR: unknown option --with-sdlabi=2.0
Try '/tmp/qemu-test/src/configure --help' for more information
# QEMU configure log Sun Feb  3 15:28:41 UTC 2019
# Configured with: '/tmp/qemu-test/src/configure' '--enable-werror' '--target-list=x86_64-softmmu,aarch64-softmmu' '--prefix=/tmp/qemu-test/install' '--python=/usr/bin/python3' '--cross-prefix=x86_64-w64-mingw32-' '--enable-trace-backends=simple' '--enable-gnutls' '--enable-nettle' '--enable-curl' '--enable-vnc' '--enable-bzip2' '--enable-guest-agent' '--with-sdlabi=2.0'
---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 634 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __linux__ not defined
 #error __linux__ not defined
  ^~~~~

---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 686 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __i386__ not defined
 #error __i386__ not defined
  ^~~~~

---
funcs: do_compiler do_cc compile_object check_define main
lines: 92 122 617 689 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __ILP32__ not defined
 #error __ILP32__ not defined
  ^~~~~

---
lines: 92 128 920 0
x86_64-w64-mingw32-gcc -mthreads -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -g -liberty
/usr/lib/gcc/x86_64-w64-mingw32/8.2.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -liberty
collect2: error: ld returned 1 exit status
Failed to run 'configure'
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 563, in <module>


The full log is available at
http://patchew.org/logs/1548942405-760115-1-git-send-email-andrey.shinkevich@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Vladimir Sementsov-Ogievskiy]

01.02.2019 21:39, Markus Armbruster wrote:
> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> 
>> In the 'Format specific information' section of the 'qemu-img info'
>> command output, the supplemental information about existing QCOW2
>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>
>> image: /vz/vmprivate/VM1/harddisk.hdd
>> file format: qcow2
>> virtual size: 64G (68719476736 bytes)
>> disk size: 3.0M
>> cluster_size: 1048576
>> Format specific information:
>>      compat: 1.1
>>      lazy refcounts: true
>>      bitmaps:
>>          [0]:
>>              flags:
>>                  [0]: in-use
>>                  [1]: auto
>>              name: back-up1
>>              unknown flags: 4
>>              granularity: 65536
>>          [1]:
>>              flags:
>>                  [0]: in-use
>>                  [1]: auto
>>              name: back-up2
>>              unknown flags: 8
>>              granularity: 65536
>>      refcount bits: 16
>>      corrupt: false
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
> [...]
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 91685be..271e0df 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -69,6 +69,8 @@
>>   # @encrypt: details about encryption parameters; only set if image
>>   #           is encrypted (since 2.10)
>>   #
>> +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +79,8 @@
>>         '*lazy-refcounts': 'bool',
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> +      '*bitmaps': ['Qcow2BitmapInfo']
>>     } }
>>   
>>   ##
>> @@ -454,6 +457,41 @@
>>              'status': 'DirtyBitmapStatus'} }
>>   
>>   ##
>> +# @Qcow2BitmapInfoFlags:
>> +#
>> +# An enumeration of flags that a bitmap can report to the user.
>> +#
>> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
> 
> I doubt the casual reader could guess the meaning from the name.  What
> about @dirty?

I'd prefer to keep it matching Qcow2 format specification. This Info aims
to export qcow2 specifics as is. And Qcow2 spec is not something qemu-internal,
why is it worse than QQPI, and why to have different name for the same thing
in different parts of spec?

We may instead rephrase it like

The bitmap is possibly in use by other process or was not saved correctly (and
therefore this flag was not cleared). The data of the bitmap may be inconsistent.

to reflect in-use name of the flag..


And, if we change it to @dirty, we at least should add a comment that this field
matches IN_USE flag of qcow2 format.

> 
>> +#
>> +# @auto: The bitmap must reflect all changes of the virtual disk by any
>> +#        application that would write to this qcow2 file.
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'enum': 'Qcow2BitmapInfoFlags',
>> +  'data': ['in-use', 'auto'] }
>> +
>> +##
>> +# @Qcow2BitmapInfo:
>> +#
>> +# Qcow2 bitmap information.
>> +#
>> +# @name: the name of the bitmap
>> +#
>> +# @granularity: granularity of the bitmap in bytes
>> +#
>> +# @flags: recognized flags of the bitmap
>> +#
>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
> 
> Intended use cases for @unknown-flags?
> 
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'Qcow2BitmapInfo',
>> +  'data': {'name': 'str', 'granularity': 'uint32',
>> +           'flags': ['Qcow2BitmapInfoFlags'],
>> +           '*unknown-flags': 'uint32' } }
>> +
>> +##
>>   # @BlockLatencyHistogramInfo:
>>   #
>>   # Block latency histogram.


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

01.02.2019 20:14, Kevin Wolf wrote:
> Am 31.01.2019 um 14:46 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>
> 
> 239 is already taken, so please rename this one. I usually search the
> mailing list for "239.out" etc. until I find a number that nobody has
> used even in a patch series yet.
> 
> Kevin
> 

Interesting, wasn't it considered to give meaningful names to iotest files?
like migrate.bitmaps or qemu-img.qcow2-bitmaps, namespace.test? And then
we'll be able to ./check -qcow2 qemu-img.*. And all these conflicts in list
will be resolved

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 04.02.2019 um 08:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 01.02.2019 20:14, Kevin Wolf wrote:
> > Am 31.01.2019 um 14:46 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>
> > 
> > 239 is already taken, so please rename this one. I usually search the
> > mailing list for "239.out" etc. until I find a number that nobody has
> > used even in a patch series yet.
> > 
> > Kevin
> > 
> 
> Interesting, wasn't it considered to give meaningful names to iotest files?
> like migrate.bitmaps or qemu-img.qcow2-bitmaps, namespace.test? And then
> we'll be able to ./check -qcow2 qemu-img.*. And all these conflicts in list
> will be resolved

You're not the first one to suggest this, and I don't think people ever
fundamentally objected to the idea. But noone implemented it either -
the current system is ugly, but it does kind of work in practice and
renaming files in the repo always comes with some cost like 'git blame'
and 'git log' stopping at the rename by default.

Kevin

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

[Kevin Wolf]

Am 01.02.2019 um 19:39 hat Markus Armbruster geschrieben:
> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> 
> > In the 'Format specific information' section of the 'qemu-img info'
> > command output, the supplemental information about existing QCOW2
> > bitmaps will be shown, such as a bitmap name, flags and granularity:
> >
> > image: /vz/vmprivate/VM1/harddisk.hdd
> > file format: qcow2
> > virtual size: 64G (68719476736 bytes)
> > disk size: 3.0M
> > cluster_size: 1048576
> > Format specific information:
> >     compat: 1.1
> >     lazy refcounts: true
> >     bitmaps:
> >         [0]:
> >             flags:
> >                 [0]: in-use
> >                 [1]: auto
> >             name: back-up1
> >             unknown flags: 4
> >             granularity: 65536
> >         [1]:
> >             flags:
> >                 [0]: in-use
> >                 [1]: auto
> >             name: back-up2
> >             unknown flags: 8
> >             granularity: 65536
> >     refcount bits: 16
> >     corrupt: false
> >
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 91685be..271e0df 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -69,6 +69,8 @@
> >  # @encrypt: details about encryption parameters; only set if image
> >  #           is encrypted (since 2.10)
> >  #
> > +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
> > +#
> >  # Since: 1.7
> >  ##
> >  { 'struct': 'ImageInfoSpecificQCow2',
> > @@ -77,7 +79,8 @@
> >        '*lazy-refcounts': 'bool',
> >        '*corrupt': 'bool',
> >        'refcount-bits': 'int',
> > -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> > +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> > +      '*bitmaps': ['Qcow2BitmapInfo']
> >    } }
> >  
> >  ##
> > @@ -454,6 +457,41 @@
> >             'status': 'DirtyBitmapStatus'} }
> >  
> >  ##
> > +# @Qcow2BitmapInfoFlags:
> > +#
> > +# An enumeration of flags that a bitmap can report to the user.
> > +#
> > +# @in-use: The bitmap was not saved correctly and may be inconsistent.
> 
> I doubt the casual reader could guess the meaning from the name.  What
> about @dirty?

Feels like overloading the word "dirty" in the context of "dirty
bitmaps". This might easily lead to misinterpretation as "this bitmap
marks some clusters as dirty" rather than "this bitmap might be
inconsistent".

Kevin

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 2/1/19 1:23 PM, Markus Armbruster wrote:
>
>>>>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
>>>>
>>>> Intended use cases for @unknown-flags?
>>>
>>> The qcow2 spec defines bit 2 extra_data_compatible; and also leaves the
>>> door open for future extensions that may define other bits. If a new
>>> version of qemu (or some non-qemu qcow2 creation app) creates an image
>>> with additional feature bits set, THIS version of qemu doesn't know what
>>> name to give those bits, but can still inform the user that those bits
>>> are set via this field. It will be omitted for all images created by
>>> this version of qemu.
>> 
>> What would QMP clients do with this information?
>
> 'qemu-img info' is the intended QMP client; and it will print the
> unknown-flags along with everything else. In other words, we're trying
> to make qemu-img become useful for inspecting as much useful information
> as possible from an image with unknown origins.  Knowing that an image
> uses bitmaps with flags unknown to THIS version of qemu is a good
> indication that you should be careful about using/altering the image
> without first upgrading qemu, as it may destroy data that some other
> product desires to utilize.

Okay.  Now work that into the documentation :)

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.02.2019 um 19:39 hat Markus Armbruster geschrieben:
>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
>> 
>> > In the 'Format specific information' section of the 'qemu-img info'
>> > command output, the supplemental information about existing QCOW2
>> > bitmaps will be shown, such as a bitmap name, flags and granularity:
>> >
>> > image: /vz/vmprivate/VM1/harddisk.hdd
>> > file format: qcow2
>> > virtual size: 64G (68719476736 bytes)
>> > disk size: 3.0M
>> > cluster_size: 1048576
>> > Format specific information:
>> >     compat: 1.1
>> >     lazy refcounts: true
>> >     bitmaps:
>> >         [0]:
>> >             flags:
>> >                 [0]: in-use
>> >                 [1]: auto
>> >             name: back-up1
>> >             unknown flags: 4
>> >             granularity: 65536
>> >         [1]:
>> >             flags:
>> >                 [0]: in-use
>> >                 [1]: auto
>> >             name: back-up2
>> >             unknown flags: 8
>> >             granularity: 65536
>> >     refcount bits: 16
>> >     corrupt: false
>> >
>> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> > ---
>> [...]
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 91685be..271e0df 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -69,6 +69,8 @@
>> >  # @encrypt: details about encryption parameters; only set if image
>> >  #           is encrypted (since 2.10)
>> >  #
>> > +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
>> > +#
>> >  # Since: 1.7
>> >  ##
>> >  { 'struct': 'ImageInfoSpecificQCow2',
>> > @@ -77,7 +79,8 @@
>> >        '*lazy-refcounts': 'bool',
>> >        '*corrupt': 'bool',
>> >        'refcount-bits': 'int',
>> > -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> > +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> > +      '*bitmaps': ['Qcow2BitmapInfo']
>> >    } }
>> >  
>> >  ##
>> > @@ -454,6 +457,41 @@
>> >             'status': 'DirtyBitmapStatus'} }
>> >  
>> >  ##
>> > +# @Qcow2BitmapInfoFlags:
>> > +#
>> > +# An enumeration of flags that a bitmap can report to the user.
>> > +#
>> > +# @in-use: The bitmap was not saved correctly and may be inconsistent.
>> 
>> I doubt the casual reader could guess the meaning from the name.  What
>> about @dirty?
>
> Feels like overloading the word "dirty" in the context of "dirty
> bitmaps". This might easily lead to misinterpretation as "this bitmap
> marks some clusters as dirty" rather than "this bitmap might be
> inconsistent".

Call it @possibly-inconsistent then?

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

[Eric Blake]

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

On 2/4/19 1:49 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +++ b/qapi/block-core.json

>>>   ##
>>> +# @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.
>>
>> I doubt the casual reader could guess the meaning from the name.  What
>> about @dirty?
> 
> I'd prefer to keep it matching Qcow2 format specification. This Info aims
> to export qcow2 specifics as is. And Qcow2 spec is not something qemu-internal,

Indeed, but we CAN reword the qcow2 spec to pick more useful names, if
the existing names there are confusing.  (The name in the spec does not
matter, so much as the layout described by the name)

> why is it worse than QQPI, and why to have different name for the same thing
> in different parts of spec?
> 
> We may instead rephrase it like
> 
> The bitmap is possibly in use by other process or was not saved correctly (and
> therefore this flag was not cleared). The data of the bitmap may be inconsistent.

s/other/another/

but that rewording works for me.

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

[Vladimir Sementsov-Ogievskiy]

04.02.2019 16:45, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 01.02.2019 um 19:39 hat Markus Armbruster geschrieben:
>>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
>>>
>>>> In the 'Format specific information' section of the 'qemu-img info'
>>>> command output, the supplemental information about existing QCOW2
>>>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>>>
>>>> image: /vz/vmprivate/VM1/harddisk.hdd
>>>> file format: qcow2
>>>> virtual size: 64G (68719476736 bytes)
>>>> disk size: 3.0M
>>>> cluster_size: 1048576
>>>> Format specific information:
>>>>      compat: 1.1
>>>>      lazy refcounts: true
>>>>      bitmaps:
>>>>          [0]:
>>>>              flags:
>>>>                  [0]: in-use
>>>>                  [1]: auto
>>>>              name: back-up1
>>>>              unknown flags: 4
>>>>              granularity: 65536
>>>>          [1]:
>>>>              flags:
>>>>                  [0]: in-use
>>>>                  [1]: auto
>>>>              name: back-up2
>>>>              unknown flags: 8
>>>>              granularity: 65536
>>>>      refcount bits: 16
>>>>      corrupt: false
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>> [...]
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 91685be..271e0df 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -69,6 +69,8 @@
>>>>   # @encrypt: details about encryption parameters; only set if image
>>>>   #           is encrypted (since 2.10)
>>>>   #
>>>> +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>>> +#
>>>>   # Since: 1.7
>>>>   ##
>>>>   { 'struct': 'ImageInfoSpecificQCow2',
>>>> @@ -77,7 +79,8 @@
>>>>         '*lazy-refcounts': 'bool',
>>>>         '*corrupt': 'bool',
>>>>         'refcount-bits': 'int',
>>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> +      '*bitmaps': ['Qcow2BitmapInfo']
>>>>     } }
>>>>   
>>>>   ##
>>>> @@ -454,6 +457,41 @@
>>>>              'status': 'DirtyBitmapStatus'} }
>>>>   
>>>>   ##
>>>> +# @Qcow2BitmapInfoFlags:
>>>> +#
>>>> +# An enumeration of flags that a bitmap can report to the user.
>>>> +#
>>>> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
>>>
>>> I doubt the casual reader could guess the meaning from the name.  What
>>> about @dirty?
>>
>> Feels like overloading the word "dirty" in the context of "dirty
>> bitmaps". This might easily lead to misinterpretation as "this bitmap
>> marks some clusters as dirty" rather than "this bitmap might be
>> inconsistent".
> 
> Call it @possibly-inconsistent then?
> 

If you run qmp query-block while vm is running, all bitmaps in image will
be in use. And in this context, in-use seems more appropriate than
possibly-inconsistent. And in-use has less interference with the fact that
actual bitmaps now are in-RAM BdrvDirtyBitmap structures which are actually
consistent (and also shown by query-block)

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

04.02.2019 16:05, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 2/1/19 1:23 PM, Markus Armbruster wrote:
>>
>>>>>> +# @unknown-flags: any remaining flags not recognized by the current qemu version
>>>>>
>>>>> Intended use cases for @unknown-flags?
>>>>
>>>> The qcow2 spec defines bit 2 extra_data_compatible; and also leaves the
>>>> door open for future extensions that may define other bits. If a new
>>>> version of qemu (or some non-qemu qcow2 creation app) creates an image
>>>> with additional feature bits set, THIS version of qemu doesn't know what
>>>> name to give those bits, but can still inform the user that those bits
>>>> are set via this field. It will be omitted for all images created by
>>>> this version of qemu.
>>>
>>> What would QMP clients do with this information?
>>
>> 'qemu-img info' is the intended QMP client; and it will print the
>> unknown-flags along with everything else. In other words, we're trying
>> to make qemu-img become useful for inspecting as much useful information
>> as possible from an image with unknown origins.  Knowing that an image
>> uses bitmaps with flags unknown to THIS version of qemu is a good
>> indication that you should be careful about using/altering the image
>> without first upgrading qemu, as it may destroy data that some other
>> product desires to utilize.
> 
> Okay.  Now work that into the documentation :)
> 

I'll try:

@unknown-flags: any flags except 'in-use' and 'auto' set for the bitmap.
                 Presence of non-zero @unknown-flags definitely shows that
                 image was created by newer version of software (which
                 supports more bitmap flags) or corrupted. Image can't be
                 used for r-w with non-zero @unknown-flags in any bitmap.


-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 2/4/19 10:03 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>> What would QMP clients do with this information?
>>>
>>> 'qemu-img info' is the intended QMP client; and it will print the
>>> unknown-flags along with everything else. In other words, we're trying
>>> to make qemu-img become useful for inspecting as much useful information
>>> as possible from an image with unknown origins.  Knowing that an image
>>> uses bitmaps with flags unknown to THIS version of qemu is a good
>>> indication that you should be careful about using/altering the image
>>> without first upgrading qemu, as it may destroy data that some other
>>> product desires to utilize.
>>
>> Okay.  Now work that into the documentation :)
>>
> 
> I'll try:
> 
> @unknown-flags: any flags except 'in-use' and 'auto' set for the bitmap.
>                  Presence of non-zero @unknown-flags definitely shows that
>                  image was created by newer version of software (which
>                  supports more bitmap flags) or corrupted. Image can't be
>                  used for r-w with non-zero @unknown-flags in any bitmap.

The grammar still feels awkward.  Maybe:

@unknown-flags: If present, this is the value of additional bitmap flags
that were set in the image but not reported in @flags, implying that the
image was created with a newer version of software (that supports new
flags) or has been corrupted. Modifying the image without upgrading qemu
first will likely break the contents of the bitmap.

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

[Vladimir Sementsov-Ogievskiy]

04.02.2019 19:24, Eric Blake wrote:
> On 2/4/19 10:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>>> What would QMP clients do with this information?
>>>>
>>>> 'qemu-img info' is the intended QMP client; and it will print the
>>>> unknown-flags along with everything else. In other words, we're trying
>>>> to make qemu-img become useful for inspecting as much useful information
>>>> as possible from an image with unknown origins.  Knowing that an image
>>>> uses bitmaps with flags unknown to THIS version of qemu is a good
>>>> indication that you should be careful about using/altering the image
>>>> without first upgrading qemu, as it may destroy data that some other
>>>> product desires to utilize.
>>>
>>> Okay.  Now work that into the documentation :)
>>>
>>
>> I'll try:
>>
>> @unknown-flags: any flags except 'in-use' and 'auto' set for the bitmap.
>>                   Presence of non-zero @unknown-flags definitely shows that
>>                   image was created by newer version of software (which
>>                   supports more bitmap flags) or corrupted. Image can't be
>>                   used for r-w with non-zero @unknown-flags in any bitmap.
> 
> The grammar still feels awkward.  Maybe:
> 
> @unknown-flags: If present, this is the value of additional bitmap flags
> that were set in the image but not reported in @flags, implying that the
> image was created with a newer version of software (that supports new
> flags) or has been corrupted. Modifying the image without upgrading qemu
> first will likely break the contents of the bitmap.
> 

Thank you!

Oops, hm, I now doubt, how Andrey's patches work, if bitmap_list_load function
fails when unknown flags are found. So, can we really show unknown-flags,
or we'll fail if there any?

And if we fail (I hope we fail) on unknown flags, than we don't need this
field :).

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

04.02.2019 19:35, Vladimir Sementsov-Ogievskiy wrote:
> 04.02.2019 19:24, Eric Blake wrote:
>> On 2/4/19 10:03 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>>>> What would QMP clients do with this information?
>>>>>
>>>>> 'qemu-img info' is the intended QMP client; and it will print the
>>>>> unknown-flags along with everything else. In other words, we're trying
>>>>> to make qemu-img become useful for inspecting as much useful information
>>>>> as possible from an image with unknown origins.  Knowing that an image
>>>>> uses bitmaps with flags unknown to THIS version of qemu is a good
>>>>> indication that you should be careful about using/altering the image
>>>>> without first upgrading qemu, as it may destroy data that some other
>>>>> product desires to utilize.
>>>>
>>>> Okay.  Now work that into the documentation :)
>>>>
>>>
>>> I'll try:
>>>
>>> @unknown-flags: any flags except 'in-use' and 'auto' set for the bitmap.
>>>                   Presence of non-zero @unknown-flags definitely shows that
>>>                   image was created by newer version of software (which
>>>                   supports more bitmap flags) or corrupted. Image can't be
>>>                   used for r-w with non-zero @unknown-flags in any bitmap.
>>
>> The grammar still feels awkward.  Maybe:
>>
>> @unknown-flags: If present, this is the value of additional bitmap flags
>> that were set in the image but not reported in @flags, implying that the
>> image was created with a newer version of software (that supports new
>> flags) or has been corrupted. Modifying the image without upgrading qemu
>> first will likely break the contents of the bitmap.
>>
> 
> Thank you!
> 
> Oops, hm, I now doubt, how Andrey's patches work, if bitmap_list_load function
> fails when unknown flags are found. So, can we really show unknown-flags,
> or we'll fail if there any?
> 
> And if we fail (I hope we fail) on unknown flags, than we don't need this
> field :).
> 

But we can update bitmap_list_load, to have a flag, and show bitmaps with
unknown flags in qemu-img info.. And we have to somehow not fail on open before
it. I'm very sorry that I understand it all only now :(.

So, do we really need unknown-flags field? If yes, may be, do it as a next step?
And proceed now without them?


-- 
Best regards,
Vladimir

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

[Eric Blake]

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

On 2/4/19 10:46 AM, Vladimir Sementsov-Ogievskiy wrote:

>>
>> Oops, hm, I now doubt, how Andrey's patches work, if bitmap_list_load function
>> fails when unknown flags are found. So, can we really show unknown-flags,
>> or we'll fail if there any?
>>
>> And if we fail (I hope we fail) on unknown flags, than we don't need this
>> field :).
>>
> 
> But we can update bitmap_list_load, to have a flag, and show bitmaps with
> unknown flags in qemu-img info.. And we have to somehow not fail on open before
> it. I'm very sorry that I understand it all only now :(.
> 
> So, do we really need unknown-flags field? If yes, may be, do it as a next step?
> And proceed now without them?

Good point about not even being able to open an image with unknown flags
to be able to report on it (qemu-img check might want to be able to do
it, but if qemu-img info refuses to open the image because of unknown
flags, that's acceptable).  I'm good with your idea of skipping
unknown-flags for now, and adding it in a later patch if it proves
worthwhile.

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

[Eric Blake]

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

On 2/4/19 11:33 AM, Eric Blake wrote:
> On 2/4/19 10:46 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>
>>> Oops, hm, I now doubt, how Andrey's patches work, if bitmap_list_load function
>>> fails when unknown flags are found. So, can we really show unknown-flags,
>>> or we'll fail if there any?
>>>
>>> And if we fail (I hope we fail) on unknown flags, than we don't need this
>>> field :).
>>>
>>
>> But we can update bitmap_list_load, to have a flag, and show bitmaps with
>> unknown flags in qemu-img info.. And we have to somehow not fail on open before
>> it. I'm very sorry that I understand it all only now :(.
>>
>> So, do we really need unknown-flags field? If yes, may be, do it as a next step?
>> And proceed now without them?
> 
> Good point about not even being able to open an image with unknown flags
> to be able to report on it (qemu-img check might want to be able to do
> it, but if qemu-img info refuses to open the image because of unknown
> flags, that's acceptable).  I'm good with your idea of skipping
> unknown-flags for now, and adding it in a later patch if it proves
> worthwhile.

It may also be worth an addition to iotests that intentionally corrupts
an image with persistent bitmaps to set an unknown bit, just to see how
qemu reacts to such an image.

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

[Kevin Wolf]

Am 04.02.2019 um 16:36 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.02.2019 16:45, Markus Armbruster wrote:
> > Kevin Wolf <kwolf@redhat.com> writes:
> > 
> >> Am 01.02.2019 um 19:39 hat Markus Armbruster geschrieben:
> >>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> >>>
> >>>> In the 'Format specific information' section of the 'qemu-img info'
> >>>> command output, the supplemental information about existing QCOW2
> >>>> bitmaps will be shown, such as a bitmap name, flags and granularity:
> >>>>
> >>>> image: /vz/vmprivate/VM1/harddisk.hdd
> >>>> file format: qcow2
> >>>> virtual size: 64G (68719476736 bytes)
> >>>> disk size: 3.0M
> >>>> cluster_size: 1048576
> >>>> Format specific information:
> >>>>      compat: 1.1
> >>>>      lazy refcounts: true
> >>>>      bitmaps:
> >>>>          [0]:
> >>>>              flags:
> >>>>                  [0]: in-use
> >>>>                  [1]: auto
> >>>>              name: back-up1
> >>>>              unknown flags: 4
> >>>>              granularity: 65536
> >>>>          [1]:
> >>>>              flags:
> >>>>                  [0]: in-use
> >>>>                  [1]: auto
> >>>>              name: back-up2
> >>>>              unknown flags: 8
> >>>>              granularity: 65536
> >>>>      refcount bits: 16
> >>>>      corrupt: false
> >>>>
> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> >>>> ---
> >>> [...]
> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>>> index 91685be..271e0df 100644
> >>>> --- a/qapi/block-core.json
> >>>> +++ b/qapi/block-core.json
> >>>> @@ -69,6 +69,8 @@
> >>>>   # @encrypt: details about encryption parameters; only set if image
> >>>>   #           is encrypted (since 2.10)
> >>>>   #
> >>>> +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
> >>>> +#
> >>>>   # Since: 1.7
> >>>>   ##
> >>>>   { 'struct': 'ImageInfoSpecificQCow2',
> >>>> @@ -77,7 +79,8 @@
> >>>>         '*lazy-refcounts': 'bool',
> >>>>         '*corrupt': 'bool',
> >>>>         'refcount-bits': 'int',
> >>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> >>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> >>>> +      '*bitmaps': ['Qcow2BitmapInfo']
> >>>>     } }
> >>>>   
> >>>>   ##
> >>>> @@ -454,6 +457,41 @@
> >>>>              'status': 'DirtyBitmapStatus'} }
> >>>>   
> >>>>   ##
> >>>> +# @Qcow2BitmapInfoFlags:
> >>>> +#
> >>>> +# An enumeration of flags that a bitmap can report to the user.
> >>>> +#
> >>>> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
> >>>
> >>> I doubt the casual reader could guess the meaning from the name.  What
> >>> about @dirty?
> >>
> >> Feels like overloading the word "dirty" in the context of "dirty
> >> bitmaps". This might easily lead to misinterpretation as "this bitmap
> >> marks some clusters as dirty" rather than "this bitmap might be
> >> inconsistent".
> > 
> > Call it @possibly-inconsistent then?
> > 
> 
> If you run qmp query-block while vm is running, all bitmaps in image will
> be in use. And in this context, in-use seems more appropriate than
> possibly-inconsistent. And in-use has less interference with the fact that
> actual bitmaps now are in-RAM BdrvDirtyBitmap structures which are actually
> consistent (and also shown by query-block)

Good point, but then this description isn't correct either:

    @in-use: The bitmap was not saved correctly and may be inconsistent.

Maybe what needs to change is the description, not the name?

Kevin

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

[Vladimir Sementsov-Ogievskiy]

05.02.2019 13:00, Kevin Wolf wrote:
> Am 04.02.2019 um 16:36 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 04.02.2019 16:45, Markus Armbruster wrote:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>>> Am 01.02.2019 um 19:39 hat Markus Armbruster geschrieben:
>>>>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
>>>>>
>>>>>> In the 'Format specific information' section of the 'qemu-img info'
>>>>>> command output, the supplemental information about existing QCOW2
>>>>>> bitmaps will be shown, such as a bitmap name, flags and granularity:
>>>>>>
>>>>>> image: /vz/vmprivate/VM1/harddisk.hdd
>>>>>> file format: qcow2
>>>>>> virtual size: 64G (68719476736 bytes)
>>>>>> disk size: 3.0M
>>>>>> cluster_size: 1048576
>>>>>> Format specific information:
>>>>>>       compat: 1.1
>>>>>>       lazy refcounts: true
>>>>>>       bitmaps:
>>>>>>           [0]:
>>>>>>               flags:
>>>>>>                   [0]: in-use
>>>>>>                   [1]: auto
>>>>>>               name: back-up1
>>>>>>               unknown flags: 4
>>>>>>               granularity: 65536
>>>>>>           [1]:
>>>>>>               flags:
>>>>>>                   [0]: in-use
>>>>>>                   [1]: auto
>>>>>>               name: back-up2
>>>>>>               unknown flags: 8
>>>>>>               granularity: 65536
>>>>>>       refcount bits: 16
>>>>>>       corrupt: false
>>>>>>
>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>> ---
>>>>> [...]
>>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>>> index 91685be..271e0df 100644
>>>>>> --- a/qapi/block-core.json
>>>>>> +++ b/qapi/block-core.json
>>>>>> @@ -69,6 +69,8 @@
>>>>>>    # @encrypt: details about encryption parameters; only set if image
>>>>>>    #           is encrypted (since 2.10)
>>>>>>    #
>>>>>> +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>>>>> +#
>>>>>>    # Since: 1.7
>>>>>>    ##
>>>>>>    { 'struct': 'ImageInfoSpecificQCow2',
>>>>>> @@ -77,7 +79,8 @@
>>>>>>          '*lazy-refcounts': 'bool',
>>>>>>          '*corrupt': 'bool',
>>>>>>          'refcount-bits': 'int',
>>>>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>>>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>>>> +      '*bitmaps': ['Qcow2BitmapInfo']
>>>>>>      } }
>>>>>>    
>>>>>>    ##
>>>>>> @@ -454,6 +457,41 @@
>>>>>>               'status': 'DirtyBitmapStatus'} }
>>>>>>    
>>>>>>    ##
>>>>>> +# @Qcow2BitmapInfoFlags:
>>>>>> +#
>>>>>> +# An enumeration of flags that a bitmap can report to the user.
>>>>>> +#
>>>>>> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
>>>>>
>>>>> I doubt the casual reader could guess the meaning from the name.  What
>>>>> about @dirty?
>>>>
>>>> Feels like overloading the word "dirty" in the context of "dirty
>>>> bitmaps". This might easily lead to misinterpretation as "this bitmap
>>>> marks some clusters as dirty" rather than "this bitmap might be
>>>> inconsistent".
>>>
>>> Call it @possibly-inconsistent then?
>>>
>>
>> If you run qmp query-block while vm is running, all bitmaps in image will
>> be in use. And in this context, in-use seems more appropriate than
>> possibly-inconsistent. And in-use has less interference with the fact that
>> actual bitmaps now are in-RAM BdrvDirtyBitmap structures which are actually
>> consistent (and also shown by query-block)
> 
> Good point, but then this description isn't correct either:
> 
>      @in-use: The bitmap was not saved correctly and may be inconsistent.
> 
> Maybe what needs to change is the description, not the name?
> 
> Kevin
> 

How about the following:

@in-use: This flag is set while bitmap is in use by software and it's data in
qcow2 image may be out of date when actual bitmap data is managed by software.
Presence of this flag in offline image means that bitmap was not saved correctly
after last usage and it's data may be inconsistent.


-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 05.02.2019 um 14:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 05.02.2019 13:00, Kevin Wolf wrote:
> > Am 04.02.2019 um 16:36 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 04.02.2019 16:45, Markus Armbruster wrote:
> >>> Kevin Wolf <kwolf@redhat.com> writes:
> >>>
> >>>> Am 01.02.2019 um 19:39 hat Markus Armbruster geschrieben:
> >>>>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> >>>>>
> >>>>>> In the 'Format specific information' section of the 'qemu-img info'
> >>>>>> command output, the supplemental information about existing QCOW2
> >>>>>> bitmaps will be shown, such as a bitmap name, flags and granularity:
> >>>>>>
> >>>>>> image: /vz/vmprivate/VM1/harddisk.hdd
> >>>>>> file format: qcow2
> >>>>>> virtual size: 64G (68719476736 bytes)
> >>>>>> disk size: 3.0M
> >>>>>> cluster_size: 1048576
> >>>>>> Format specific information:
> >>>>>>       compat: 1.1
> >>>>>>       lazy refcounts: true
> >>>>>>       bitmaps:
> >>>>>>           [0]:
> >>>>>>               flags:
> >>>>>>                   [0]: in-use
> >>>>>>                   [1]: auto
> >>>>>>               name: back-up1
> >>>>>>               unknown flags: 4
> >>>>>>               granularity: 65536
> >>>>>>           [1]:
> >>>>>>               flags:
> >>>>>>                   [0]: in-use
> >>>>>>                   [1]: auto
> >>>>>>               name: back-up2
> >>>>>>               unknown flags: 8
> >>>>>>               granularity: 65536
> >>>>>>       refcount bits: 16
> >>>>>>       corrupt: false
> >>>>>>
> >>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> >>>>>> ---
> >>>>> [...]
> >>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>>>>> index 91685be..271e0df 100644
> >>>>>> --- a/qapi/block-core.json
> >>>>>> +++ b/qapi/block-core.json
> >>>>>> @@ -69,6 +69,8 @@
> >>>>>>    # @encrypt: details about encryption parameters; only set if image
> >>>>>>    #           is encrypted (since 2.10)
> >>>>>>    #
> >>>>>> +# @bitmaps: A list of qcow2 bitmap details (since 4.0)
> >>>>>> +#
> >>>>>>    # Since: 1.7
> >>>>>>    ##
> >>>>>>    { 'struct': 'ImageInfoSpecificQCow2',
> >>>>>> @@ -77,7 +79,8 @@
> >>>>>>          '*lazy-refcounts': 'bool',
> >>>>>>          '*corrupt': 'bool',
> >>>>>>          'refcount-bits': 'int',
> >>>>>> -      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> >>>>>> +      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> >>>>>> +      '*bitmaps': ['Qcow2BitmapInfo']
> >>>>>>      } }
> >>>>>>    
> >>>>>>    ##
> >>>>>> @@ -454,6 +457,41 @@
> >>>>>>               'status': 'DirtyBitmapStatus'} }
> >>>>>>    
> >>>>>>    ##
> >>>>>> +# @Qcow2BitmapInfoFlags:
> >>>>>> +#
> >>>>>> +# An enumeration of flags that a bitmap can report to the user.
> >>>>>> +#
> >>>>>> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
> >>>>>
> >>>>> I doubt the casual reader could guess the meaning from the name.  What
> >>>>> about @dirty?
> >>>>
> >>>> Feels like overloading the word "dirty" in the context of "dirty
> >>>> bitmaps". This might easily lead to misinterpretation as "this bitmap
> >>>> marks some clusters as dirty" rather than "this bitmap might be
> >>>> inconsistent".
> >>>
> >>> Call it @possibly-inconsistent then?
> >>>
> >>
> >> If you run qmp query-block while vm is running, all bitmaps in image will
> >> be in use. And in this context, in-use seems more appropriate than
> >> possibly-inconsistent. And in-use has less interference with the fact that
> >> actual bitmaps now are in-RAM BdrvDirtyBitmap structures which are actually
> >> consistent (and also shown by query-block)
> > 
> > Good point, but then this description isn't correct either:
> > 
> >      @in-use: The bitmap was not saved correctly and may be inconsistent.
> > 
> > Maybe what needs to change is the description, not the name?
> > 
> > Kevin
> > 
> 
> How about the following:
> 
> @in-use: This flag is set while bitmap is in use by software and it's data in
> qcow2 image may be out of date when actual bitmap data is managed by software.
> Presence of this flag in offline image means that bitmap was not saved correctly
> after last usage and it's data may be inconsistent.

Yes, I think this has the information it needs to have.

It needs some spelling and grammar fixes, but before I make them, I'll
wait if Eric would phrase it in a different way that sounds more natural
to him as a native speaker.

Kevin

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

[Eric Blake]

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

On 2/5/19 8:28 AM, Kevin Wolf wrote:

>> How about the following:
>>
>> @in-use: This flag is set while bitmap is in use by software and it's data in
>> qcow2 image may be out of date when actual bitmap data is managed by software.
>> Presence of this flag in offline image means that bitmap was not saved correctly
>> after last usage and it's data may be inconsistent.
> 
> Yes, I think this has the information it needs to have.
> 
> It needs some spelling and grammar fixes, but before I make them, I'll
> wait if Eric would phrase it in a different way that sounds more natural
> to him as a native speaker.

@in-use: This flag is set by any process actively modifying the qcow2
file, and cleared when the updated bitmap is flushed to the qcow2 image.
Presence of this flag in an offline image means that the bitmap was not
saved correctly after its last usage, and may contain inconsistent data.

-- 
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 --]