[PULL 0/5] bitmaps patches through 2021-02-12

[PULL 0/5] bitmaps patches through 2021-02-12

[Eric Blake]

The following changes since commit eac92d316351b855ba79eb374dd21cc367f1f9c1:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20210211-1' into staging (2021-02-11 19:57:50 +0000)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-bitmaps-2021-02-12

for you to fetch changes up to 934aee14d36e67468260635af61c387227cdaf78:

  block: use return status of bdrv_append() (2021-02-12 15:39:44 -0600)

----------------------------------------------------------------
bitmaps patches for 2021-02-12

- add 'transform' member to manipulate bitmaps across migration
- work towards better error handling during bdrv_open

----------------------------------------------------------------
Peter Krempa (3):
      migration: dirty-bitmap: Use struct for alias map inner members
      migration: dirty-bitmap: Allow control of bitmap persistence
      qemu-iotests: 300: Add test case for modifying persistence of bitmap

Vladimir Sementsov-Ogievskiy (2):
      block: return status from bdrv_append and friends
      block: use return status of bdrv_append()

 qapi/migration.json            | 19 ++++++++-
 include/block/block.h          | 12 +++---
 block.c                        | 64 +++++++++++++++++------------
 block/backup-top.c             | 23 +++++------
 block/commit.c                 |  6 +--
 block/mirror.c                 |  6 +--
 blockdev.c                     |  6 +--
 migration/block-dirty-bitmap.c | 62 +++++++++++++++++++++-------
 tests/test-bdrv-graph-mod.c    |  6 +--
 tests/qemu-iotests/300         | 93 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out     |  4 +-
 11 files changed, 226 insertions(+), 75 deletions(-)

-- 
2.30.1

[PULL 1/5] migration: dirty-bitmap: Use struct for alias map inner members

[Eric Blake]

From: Peter Krempa <pkrempa@redhat.com>

Currently the alias mapping hash stores just strings of the target
objects internally. In further patches we'll be adding another member
which will need to be stored in the map so pass a copy of the whole
BitmapMigrationBitmapAlias QAPI struct into the map.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Message-Id: <fc5f27e1fe16cb75e08a248c2d938de3997b9bfb.1613150869.git.pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: adjust long lines]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 migration/block-dirty-bitmap.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index c61d382be87c..b39c13ce4ebe 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -75,6 +75,8 @@
 #include "qemu/id.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-migration.h"
+#include "qapi/qapi-visit-migration.h"
+#include "qapi/clone-visitor.h"
 #include "trace.h"

 #define CHUNK_SIZE     (1 << 10)
@@ -224,6 +226,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
         AliasMapInnerNode *amin;
         GHashTable *bitmaps_map;
         const char *node_map_from, *node_map_to;
+        GDestroyNotify gdn;

         if (!id_wellformed(bmna->alias)) {
             error_setg(errp, "The node alias '%s' is not well-formed",
@@ -263,8 +266,9 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
             node_map_to = bmna->node_name;
         }

-        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
-                                            g_free, g_free);
+        gdn = (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias;
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+                                            gdn);

         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -276,7 +280,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,

         for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
             const BitmapMigrationBitmapAlias *bmba = bmbal->value;
-            const char *bmap_map_from, *bmap_map_to;
+            const char *bmap_map_from;

             if (strlen(bmba->alias) > UINT8_MAX) {
                 error_setg(errp,
@@ -293,7 +297,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,

             if (name_to_alias) {
                 bmap_map_from = bmba->name;
-                bmap_map_to = bmba->alias;

                 if (g_hash_table_contains(bitmaps_map, bmba->name)) {
                     error_setg(errp, "The bitmap '%s'/'%s' is mapped twice",
@@ -302,7 +305,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
                 }
             } else {
                 bmap_map_from = bmba->alias;
-                bmap_map_to = bmba->name;

                 if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
                     error_setg(errp, "The bitmap alias '%s'/'%s' is used twice",
@@ -311,8 +313,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
                 }
             }

-            g_hash_table_insert(bitmaps_map,
-                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
+            g_hash_table_insert(bitmaps_map, g_strdup(bmap_map_from),
+                                QAPI_CLONE(BitmapMigrationBitmapAlias, bmba));
         }
     }

@@ -538,11 +540,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
         }

         if (bitmap_aliases) {
-            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
-            if (!bitmap_alias) {
+            BitmapMigrationBitmapAlias *bmap_inner;
+
+            bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+            if (!bmap_inner) {
                 /* Skip bitmaps with no alias */
                 continue;
             }
+
+            bitmap_alias = bmap_inner->alias;
         } else {
             if (strlen(bitmap_name) > UINT8_MAX) {
                 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -1074,13 +1080,16 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,

         bitmap_name = s->bitmap_alias;
         if (!s->cancelled && bitmap_alias_map) {
-            bitmap_name = g_hash_table_lookup(bitmap_alias_map,
-                                              s->bitmap_alias);
-            if (!bitmap_name) {
+            BitmapMigrationBitmapAlias *bmap_inner;
+
+            bmap_inner = g_hash_table_lookup(bitmap_alias_map, s->bitmap_alias);
+            if (!bmap_inner) {
                 error_report("Error: Unknown bitmap alias '%s' on node "
                              "'%s' (alias '%s')", s->bitmap_alias,
                              s->bs->node_name, s->node_alias);
                 cancel_incoming_locked(s);
+            } else {
+                bitmap_name = bmap_inner->name;
             }
         }

-- 
2.30.1

[PULL 2/5] migration: dirty-bitmap: Allow control of bitmap persistence

[Eric Blake]

From: Peter Krempa <pkrempa@redhat.com>

Bitmap's source persistence is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merging bitmaps from a number of layers on the source when migrating
into a squashed image) but currently it would need to create another set
of persistent bitmaps and merge them.

This patch adds a 'transform' property to the alias map which allows
overriding the persistence of migrated bitmaps both on the source and
destination sides.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Message-Id: <b20afb675917b86f6359ac3591166ac6d4233573.1613150869.git.pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: grammar tweaks, drop dead conditional]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/migration.json            | 19 ++++++++++++++++++-
 migration/block-dirty-bitmap.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index ce14d78071a5..6e5943fbb443 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -536,6 +536,19 @@
   'data': [ 'none', 'zlib',
             { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

+##
+# @BitmapMigrationBitmapAliasTransform:
+#
+# @persistent: If present, the bitmap will be made persistent
+#              or transient depending on this parameter.
+#
+# Since: 6.0
+##
+{ 'struct': 'BitmapMigrationBitmapAliasTransform',
+  'data': {
+      '*persistent': 'bool'
+  } }
+
 ##
 # @BitmapMigrationBitmapAlias:
 #
@@ -544,12 +557,16 @@
 # @alias: An alias name for migration (for example the bitmap name on
 #         the opposite site).
 #
+# @transform: Allows the modification of the migrated bitmap.
+#             (since 6.0)
+#
 # Since: 5.2
 ##
 { 'struct': 'BitmapMigrationBitmapAlias',
   'data': {
       'name': 'str',
-      'alias': 'str'
+      'alias': 'str',
+      '*transform': 'BitmapMigrationBitmapAliasTransform'
   } }

 ##
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b39c13ce4ebe..975093610a41 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -150,6 +150,7 @@ typedef struct DBMLoadState {
     BdrvDirtyBitmap *bitmap;

     bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
+    BitmapMigrationBitmapAlias *bmap_inner;

     /*
      * cancelled
@@ -529,6 +530,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
     }

     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+        BitmapMigrationBitmapAliasTransform *bitmap_transform = NULL;
         bitmap_name = bdrv_dirty_bitmap_name(bitmap);
         if (!bitmap_name) {
             continue;
@@ -549,6 +551,9 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
             }

             bitmap_alias = bmap_inner->alias;
+            if (bmap_inner->has_transform) {
+                bitmap_transform = bmap_inner->transform;
+            }
         } else {
             if (strlen(bitmap_name) > UINT8_MAX) {
                 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -574,8 +579,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
         if (bdrv_dirty_bitmap_enabled(bitmap)) {
             dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
         }
-        if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-            dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+        if (bitmap_transform &&
+            bitmap_transform->has_persistent) {
+            if (bitmap_transform->persistent) {
+                dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+            }
+        } else {
+            if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+                dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+            }
         }

         QSIMPLEQ_INSERT_TAIL(&s->dbms_list, dbms, entry);
@@ -783,6 +795,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
     uint32_t granularity = qemu_get_be32(f);
     uint8_t flags = qemu_get_byte(f);
     LoadBitmapState *b;
+    bool persistent;

     if (s->cancelled) {
         return 0;
@@ -807,7 +820,15 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
         return -EINVAL;
     }

-    if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
+    if (s->bmap_inner &&
+        s->bmap_inner->has_transform &&
+        s->bmap_inner->transform->has_persistent) {
+        persistent = s->bmap_inner->transform->persistent;
+    } else {
+        persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+    }
+
+    if (persistent) {
         bdrv_dirty_bitmap_set_persistence(s->bitmap, true);
     }

@@ -1091,6 +1112,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
             } else {
                 bitmap_name = bmap_inner->name;
             }
+
+            s->bmap_inner = bmap_inner;
         }

         if (!s->cancelled) {
-- 
2.30.1

[PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Eric Blake]

From: Peter Krempa <pkrempa@redhat.com>

Verify that the modification of the bitmap persistence over migration
which is controlled via BitmapMigrationBitmapAliasTransform works
properly.

Based on TestCrossAliasMigration

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: Adjust test for explicit read_zeroes=False]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/300     | 93 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out |  4 +-
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 43264d883d79..63036f6a6e13 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -600,6 +600,99 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration):
         self.verify_dest_has_all_bitmaps()
         self.verify_dest_error(None)

+class TestAliasTransformMigration(TestDirtyBitmapMigration):
+    """
+    Tests the 'transform' option which modifies bitmap persistence on migration.
+    """
+
+    src_node_name = 'node-a'
+    dst_node_name = 'node-b'
+    src_bmap_name = 'bmap-a'
+    dst_bmap_name = 'bmap-b'
+
+    def setUp(self) -> None:
+        TestDirtyBitmapMigration.setUp(self)
+
+        # Now create another block device and let both have two bitmaps each
+        result = self.vm_a.qmp('blockdev-add',
+                               node_name='node-b', driver='null-co',
+                               read_zeroes=False)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm_b.qmp('blockdev-add',
+                               node_name='node-a', driver='null-co',
+                               read_zeroes=False)
+        self.assert_qmp(result, 'return', {})
+
+        bmaps_to_add = (('node-a', 'bmap-b'),
+                        ('node-b', 'bmap-a'),
+                        ('node-b', 'bmap-b'))
+
+        for (node, bmap) in bmaps_to_add:
+            result = self.vm_a.qmp('block-dirty-bitmap-add',
+                                   node=node, name=bmap)
+            self.assert_qmp(result, 'return', {})
+
+    @staticmethod
+    def transform_mapping() -> BlockBitmapMapping:
+        return [
+            {
+                'node-name': 'node-a',
+                'alias': 'node-a',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-a',
+                        'transform':
+                            {
+                                'persistent': True
+                            }
+                    },
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-b'
+                    }
+                ]
+            },
+            {
+                'node-name': 'node-b',
+                'alias': 'node-b',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-a'
+                    },
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-b'
+                    }
+                ]
+            }
+        ]
+
+    def verify_dest_bitmap_state(self) -> None:
+        bitmaps = self.vm_b.query_bitmaps()
+
+        for node in bitmaps:
+            bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for bmap in bitmaps[node]))
+
+        self.assertEqual(bitmaps,
+                         {'node-a': [('bmap-a', True), ('bmap-b', False)],
+                          'node-b': [('bmap-a', False), ('bmap-b', False)]})
+
+    def test_transform_on_src(self) -> None:
+        self.set_mapping(self.vm_a, self.transform_mapping())
+
+        self.migrate()
+        self.verify_dest_bitmap_state()
+        self.verify_dest_error(None)
+
+    def test_transform_on_dst(self) -> None:
+        self.set_mapping(self.vm_b, self.transform_mapping())
+
+        self.migrate()
+        self.verify_dest_bitmap_state()
+        self.verify_dest_error(None)

 if __name__ == '__main__':
     iotests.main(supported_protocols=['file'])
diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out
index cafb8161f7b1..12e9ab7d571b 100644
--- a/tests/qemu-iotests/300.out
+++ b/tests/qemu-iotests/300.out
@@ -1,5 +1,5 @@
-.....................................
+.......................................
 ----------------------------------------------------------------------
-Ran 37 tests
+Ran 39 tests

 OK
-- 
2.30.1

[PULL 4/5] block: return status from bdrv_append and friends

[Eric Blake]

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

The recommended use of qemu error api assumes returning status together
with setting errp and avoid void functions with errp parameter. Let's
improve bdrv_append and some friends to reduce error-propagation
overhead in further patches.

Choose int return status, because bdrv_replace_node_common() has call
to bdrv_check_update_perm(), which reports int status, which seems
correct to propagate.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210202124956.63146-2-vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block.h | 12 ++++-----
 block.c               | 58 +++++++++++++++++++++++++++----------------
 2 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 0a9f2c187cdb..2c235a29e822 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -356,10 +356,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);

 BlockDriverState *bdrv_new(void);
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
-                 Error **errp);
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
-                       Error **errp);
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+                Error **errp);
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                      Error **errp);
 BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
                                    int flags, Error **errp);

@@ -373,8 +373,8 @@ BdrvChild *bdrv_open_child(const char *filename,
                            BdrvChildRole child_role,
                            bool allow_none, Error **errp);
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
-                         Error **errp);
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+                        Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp);
 BlockDriverState *bdrv_open(const char *filename, const char *reference,
diff --git a/block.c b/block.c
index 4e52b1c588cd..fdb0261cb324 100644
--- a/block.c
+++ b/block.c
@@ -2827,14 +2827,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
  * Sets the bs->backing link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
  */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
-                         Error **errp)
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+                        Error **errp)
 {
+    int ret = 0;
     bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
         bdrv_inherits_from_recursive(backing_hd, bs);

     if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
-        return;
+        return -EPERM;
     }

     if (backing_hd) {
@@ -2853,15 +2854,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,

     bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
                                     bdrv_backing_role(bs), errp);
+    if (!bs->backing) {
+        ret = -EPERM;
+        goto out;
+    }
+
     /* If backing_hd was already part of bs's backing chain, and
      * inherits_from pointed recursively to bs then let's update it to
      * point directly to bs (else it will become NULL). */
-    if (bs->backing && update_inherits_from) {
+    if (update_inherits_from) {
         backing_hd->inherits_from = bs;
     }

 out:
     bdrv_refresh_limits(bs, NULL);
+
+    return ret;
 }

 /*
@@ -4532,9 +4540,9 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
  * With auto_skip=false the error is returned if from has a parent which should
  * not be updated.
  */
-static void bdrv_replace_node_common(BlockDriverState *from,
-                                     BlockDriverState *to,
-                                     bool auto_skip, Error **errp)
+static int bdrv_replace_node_common(BlockDriverState *from,
+                                    BlockDriverState *to,
+                                    bool auto_skip, Error **errp)
 {
     BdrvChild *c, *next;
     GSList *list = NULL, *p;
@@ -4556,11 +4564,13 @@ static void bdrv_replace_node_common(BlockDriverState *from,
             if (auto_skip) {
                 continue;
             }
+            ret = -EINVAL;
             error_setg(errp, "Should not change '%s' link to '%s'",
                        c->name, from->node_name);
             goto out;
         }
         if (c->frozen) {
+            ret = -EPERM;
             error_setg(errp, "Cannot change '%s' link to '%s'",
                        c->name, from->node_name);
             goto out;
@@ -4591,14 +4601,18 @@ static void bdrv_replace_node_common(BlockDriverState *from,

     bdrv_set_perm(to);

+    ret = 0;
+
 out:
     g_slist_free(list);
     bdrv_drained_end(from);
     bdrv_unref(from);
+
+    return ret;
 }

-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
-                       Error **errp)
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                      Error **errp)
 {
     return bdrv_replace_node_common(from, to, true, errp);
 }
@@ -4619,28 +4633,30 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
  * parents of bs_top after bdrv_append() returns. If the caller needs to keep a
  * reference of its own, it must call bdrv_ref().
  */
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
-                 Error **errp)
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+                Error **errp)
 {
-    Error *local_err = NULL;
-
-    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
+    if (ret < 0) {
         goto out;
     }

-    bdrv_replace_node(bs_top, bs_new, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    ret = bdrv_replace_node(bs_top, bs_new, errp);
+    if (ret < 0) {
         bdrv_set_backing_hd(bs_new, NULL, &error_abort);
         goto out;
     }

-    /* bs_new is now referenced by its new parents, we don't need the
-     * additional reference any more. */
+    ret = 0;
+
 out:
+    /*
+     * bs_new is now referenced by its new parents, we don't need the
+     * additional reference any more.
+     */
     bdrv_unref(bs_new);
+
+    return ret;
 }

 static void bdrv_delete(BlockDriverState *bs)
-- 
2.30.1

[PULL 5/5] block: use return status of bdrv_append()

[Eric Blake]

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

Now bdrv_append returns status and we can drop all the local_err things
around it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-Id: <20210202124956.63146-3-vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block.c                     |  6 ++----
 block/backup-top.c          | 23 +++++++++++------------
 block/commit.c              |  6 ++----
 block/mirror.c              |  6 ++----
 blockdev.c                  |  6 +++---
 tests/test-bdrv-graph-mod.c |  6 +++---
 6 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index fdb0261cb324..c682c3e3b96b 100644
--- a/block.c
+++ b/block.c
@@ -3118,7 +3118,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     int64_t total_size;
     QemuOpts *opts = NULL;
     BlockDriverState *bs_snapshot = NULL;
-    Error *local_err = NULL;
     int ret;

     /* if snapshot, we create a temporary backing file and open it
@@ -3165,9 +3164,8 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
      * order to be able to return one, we have to increase
      * bs_snapshot's refcount here */
     bdrv_ref(bs_snapshot);
-    bdrv_append(bs_snapshot, bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    ret = bdrv_append(bs_snapshot, bs, errp);
+    if (ret < 0) {
         bs_snapshot = NULL;
         goto out;
     }
diff --git a/block/backup-top.c b/block/backup-top.c
index 6e7e7bf34047..d1253e1aa63f 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -191,7 +191,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
                                          BlockCopyState **bcs,
                                          Error **errp)
 {
-    Error *local_err = NULL;
+    ERRP_GUARD();
+    int ret;
     BDRVBackupTopState *state;
     BlockDriverState *top;
     bool appended = false;
@@ -224,9 +225,9 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     bdrv_drained_begin(source);

     bdrv_ref(top);
-    bdrv_append(top, source, &local_err);
-    if (local_err) {
-        error_prepend(&local_err, "Cannot append backup-top filter: ");
+    ret = bdrv_append(top, source, errp);
+    if (ret < 0) {
+        error_prepend(errp, "Cannot append backup-top filter: ");
         goto fail;
     }
     appended = true;
@@ -236,19 +237,18 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
      * we want.
      */
     state->active = true;
-    bdrv_child_refresh_perms(top, top->backing, &local_err);
-    if (local_err) {
-        error_prepend(&local_err,
-                      "Cannot set permissions for backup-top filter: ");
+    ret = bdrv_child_refresh_perms(top, top->backing, errp);
+    if (ret < 0) {
+        error_prepend(errp, "Cannot set permissions for backup-top filter: ");
         goto fail;
     }

     state->cluster_size = cluster_size;
     state->bcs = block_copy_state_new(top->backing, state->target,
                                       cluster_size, perf->use_copy_range,
-                                      write_flags, &local_err);
-    if (local_err) {
-        error_prepend(&local_err, "Cannot create block-copy-state: ");
+                                      write_flags, errp);
+    if (!state->bcs) {
+        error_prepend(errp, "Cannot create block-copy-state: ");
         goto fail;
     }
     *bcs = state->bcs;
@@ -266,7 +266,6 @@ fail:
     }

     bdrv_drained_end(source);
-    error_propagate(errp, local_err);

     return NULL;
 }
diff --git a/block/commit.c b/block/commit.c
index 71db7ba7472e..dd9ba87349e3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -254,7 +254,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     BlockDriverState *iter;
     BlockDriverState *commit_top_bs = NULL;
     BlockDriverState *filtered_base;
-    Error *local_err = NULL;
     int64_t base_size, top_size;
     uint64_t base_perms, iter_shared_perms;
     int ret;
@@ -312,10 +311,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,

     commit_top_bs->total_sectors = top->total_sectors;

-    bdrv_append(commit_top_bs, top, &local_err);
-    if (local_err) {
+    ret = bdrv_append(commit_top_bs, top, errp);
+    if (ret < 0) {
         commit_top_bs = NULL;
-        error_propagate(errp, local_err);
         goto fail;
     }

diff --git a/block/mirror.c b/block/mirror.c
index 8e1ad6eceb57..fad270193888 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1560,7 +1560,6 @@ static BlockJob *mirror_start_job(
     BlockDriverState *mirror_top_bs;
     bool target_is_backing;
     uint64_t target_perms, target_shared_perms;
-    Error *local_err = NULL;
     int ret;

     if (granularity == 0) {
@@ -1609,12 +1608,11 @@ static BlockJob *mirror_start_job(
      * it alive until block_job_create() succeeds even if bs has no parent. */
     bdrv_ref(mirror_top_bs);
     bdrv_drained_begin(bs);
-    bdrv_append(mirror_top_bs, bs, &local_err);
+    ret = bdrv_append(mirror_top_bs, bs, errp);
     bdrv_drained_end(bs);

-    if (local_err) {
+    if (ret < 0) {
         bdrv_unref(mirror_top_bs);
-        error_propagate(errp, local_err);
         return NULL;
     }

diff --git a/blockdev.c b/blockdev.c
index b250b9b95957..cd438e60e35a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1432,6 +1432,7 @@ typedef struct ExternalSnapshotState {
 static void external_snapshot_prepare(BlkActionState *common,
                                       Error **errp)
 {
+    int ret;
     int flags = 0;
     QDict *options = NULL;
     Error *local_err = NULL;
@@ -1591,9 +1592,8 @@ static void external_snapshot_prepare(BlkActionState *common,
      * can fail, so we need to do it in .prepare; undoing it for abort is
      * always possible. */
     bdrv_ref(state->new_bs);
-    bdrv_append(state->new_bs, state->old_bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    ret = bdrv_append(state->new_bs, state->old_bs, errp);
+    if (ret < 0) {
         goto out;
     }
     state->overlay_appended = true;
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index 8cff13830e3e..c4f7d1603924 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name)
  */
 static void test_update_perm_tree(void)
 {
-    Error *local_err = NULL;
+    int ret;

     BlockBackend *root = blk_new(qemu_get_aio_context(),
                                  BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ,
@@ -114,8 +114,8 @@ static void test_update_perm_tree(void)
     bdrv_attach_child(filter, bs, "child", &child_of_bds,
                       BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);

-    bdrv_append(filter, bs, &local_err);
-    error_free_or_abort(&local_err);
+    ret = bdrv_append(filter, bs, NULL);
+    g_assert_cmpint(ret, <, 0);

     blk_unref(root);
 }
-- 
2.30.1

Re: [PULL 0/5] bitmaps patches through 2021-02-12

[Peter Maydell]

On Fri, 12 Feb 2021 at 23:24, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit eac92d316351b855ba79eb374dd21cc367f1f9c1:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20210211-1' into staging (2021-02-11 19:57:50 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/ericb.git tags/pull-bitmaps-2021-02-12
>
> for you to fetch changes up to 934aee14d36e67468260635af61c387227cdaf78:
>
>   block: use return status of bdrv_append() (2021-02-12 15:39:44 -0600)
>
> ----------------------------------------------------------------
> bitmaps patches for 2021-02-12
>
> - add 'transform' member to manipulate bitmaps across migration
> - work towards better error handling during bdrv_open
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Kevin Wolf]

Am 13.02.2021 um 00:21 hat Eric Blake geschrieben:
> From: Peter Krempa <pkrempa@redhat.com>
> 
> Verify that the modification of the bitmap persistence over migration
> which is controlled via BitmapMigrationBitmapAliasTransform works
> properly.
> 
> Based on TestCrossAliasMigration
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> [eblake: Adjust test for explicit read_zeroes=False]
> Signed-off-by: Eric Blake <eblake@redhat.com>

This breaks 297:

--- /home/kwolf/source/qemu/tests/qemu-iotests/297.out
+++ 297.out.bad
@@ -1,2 +1,8 @@
 === pylint ===
+************* Module 300
+300:605:0: C0301: Line too long (80/79) (line-too-long)
+300:677:0: C0301: Line too long (98/79) (line-too-long)
 === mypy ===
+300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
+Found 1 error in 1 file (checked 1 source file)
+

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Eric Blake]

On 2/15/21 6:31 AM, Kevin Wolf wrote:
> Am 13.02.2021 um 00:21 hat Eric Blake geschrieben:
>> From: Peter Krempa <pkrempa@redhat.com>
>>
>> Verify that the modification of the bitmap persistence over migration
>> which is controlled via BitmapMigrationBitmapAliasTransform works
>> properly.
>>
>> Based on TestCrossAliasMigration
>>
>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> [eblake: Adjust test for explicit read_zeroes=False]
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> This breaks 297:
> 
> --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out
> +++ 297.out.bad
> @@ -1,2 +1,8 @@
>  === pylint ===
> +************* Module 300
> +300:605:0: C0301: Line too long (80/79) (line-too-long)
> +300:677:0: C0301: Line too long (98/79) (line-too-long)

These two are easy fixes (add line breaks for shorter lines), but this:

>  === mypy ===
> +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
> +Found 1 error in 1 file (checked 1 source file)

is beyond my skill.  The typing at line 33:

BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]

is insufficient to allow our new 'transform' member in the new
transform_mapping() -> Block BitmapMapping near line 677:

                'bitmaps': [
                    {
                        'name': 'bmap-a',
                        'alias': 'bmap-a',
                        'transform':
                            {
                                'persistent': True
                            }
                    },

but I'm not sure how to tell python the right type it should be.  John?

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

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Kevin Wolf]

Am 15.02.2021 um 17:46 hat Eric Blake geschrieben:
> On 2/15/21 6:31 AM, Kevin Wolf wrote:
> > Am 13.02.2021 um 00:21 hat Eric Blake geschrieben:
> >> From: Peter Krempa <pkrempa@redhat.com>
> >>
> >> Verify that the modification of the bitmap persistence over migration
> >> which is controlled via BitmapMigrationBitmapAliasTransform works
> >> properly.
> >>
> >> Based on TestCrossAliasMigration
> >>
> >> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> >> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> [eblake: Adjust test for explicit read_zeroes=False]
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> > This breaks 297:
> > 
> > --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out
> > +++ 297.out.bad
> > @@ -1,2 +1,8 @@
> >  === pylint ===
> > +************* Module 300
> > +300:605:0: C0301: Line too long (80/79) (line-too-long)
> > +300:677:0: C0301: Line too long (98/79) (line-too-long)
> 
> These two are easy fixes (add line breaks for shorter lines), but this:
> 
> >  === mypy ===
> > +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
> > +Found 1 error in 1 file (checked 1 source file)
> 
> is beyond my skill.  The typing at line 33:
> 
> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
> 
> is insufficient to allow our new 'transform' member in the new
> transform_mapping() -> Block BitmapMapping near line 677:
> 
>                 'bitmaps': [
>                     {
>                         'name': 'bmap-a',
>                         'alias': 'bmap-a',
>                         'transform':
>                             {
>                                 'persistent': True
>                             }
>                     },
> 
> but I'm not sure how to tell python the right type it should be.  John?

To be honest, this looks sufficiently like JSON that I would just go for
List[Dict[str, Any]] (as long as recursive types don't work), but if you
really want to have an explicit type, I think you'd have to replace the
rightmost str with Union[str, Dict[str, bool]] to allow both.

Kevin

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Eric Blake]

On 2/15/21 11:09 AM, Kevin Wolf wrote:

>>> This breaks 297:

>>>  === mypy ===
>>> +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
>>> +Found 1 error in 1 file (checked 1 source file)
>>
>> is beyond my skill.  The typing at line 33:
>>
>> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>>
>> is insufficient to allow our new 'transform' member in the new
>> transform_mapping() -> Block BitmapMapping near line 677:
>>
>>                 'bitmaps': [
>>                     {
>>                         'name': 'bmap-a',
>>                         'alias': 'bmap-a',
>>                         'transform':
>>                             {
>>                                 'persistent': True
>>                             }
>>                     },
>>
>> but I'm not sure how to tell python the right type it should be.  John?
> 
> To be honest, this looks sufficiently like JSON that I would just go for
> List[Dict[str, Any]] (as long as recursive types don't work), but if you
> really want to have an explicit type, I think you'd have to replace the
> rightmost str with Union[str, Dict[str, bool]] to allow both.

Indeed, I played with it before reading your response, and came up with
this.  Shall I turn it into a formal patch?

diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
index 63036f6a6e13..7501bd1018e2 100755
--- i/tests/qemu-iotests/300
+++ w/tests/qemu-iotests/300
@@ -30,7 +30,10 @@ import iotests
 # pylint: disable=wrong-import-order
 import qemu

-BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
+BlockBitmapMapping = List[Dict[str,
+                               Union[str,
+                                     List[Dict[str,
+                                               Union[str, Dict[str,
bool]]]]]]]

 mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')

@@ -602,7 +605,8 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration):

 class TestAliasTransformMigration(TestDirtyBitmapMigration):
     """
-    Tests the 'transform' option which modifies bitmap persistence on
migration.
+    Tests the 'transform' option which modifies bitmap persistence on
+    migration.
     """

     src_node_name = 'node-a'
@@ -674,7 +678,8 @@ class
TestAliasTransformMigration(TestDirtyBitmapMigration):
         bitmaps = self.vm_b.query_bitmaps()

         for node in bitmaps:
-            bitmaps[node] = sorted(((bmap['name'], bmap['persistent'])
for bmap in bitmaps[node]))
+            bitmaps[node] = sorted(((bmap['name'], bmap['persistent'])
+                                    for bmap in bitmaps[node]))

         self.assertEqual(bitmaps,
                          {'node-a': [('bmap-a', True), ('bmap-b', False)],


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

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[John Snow]

On 2/15/21 1:25 PM, Eric Blake wrote:
> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
> +BlockBitmapMapping = List[Dict[str,
> +                               Union[str,
> +                                     List[Dict[str,
> +                                               Union[str, Dict[str,
> bool]]]]]]]

That looks *very* beefy.

Is the Union because that union is valid for every key, or because every 
key has a potentially different value that is specific to that key?

if it's the latter, I'd ditch the Union and just go with:

Dict[str, object], or
Dict[str, Any]

object: will allow any type, but keeps strict checking enabled. If you 
try to use that value later on without a cast, mypy will warn you if you 
are using it in a manner not guaranteed by the "object" type. Can be 
useful if you are passing values to a function that already does RTTI to 
determine behavior.

Any: Also allows any type, but enables gradual typing. If you later 
"assume" the type of this value, mypy will say nothing. Can be useful 
when you've just got a job to do and the right tool would have been a 
recursive type or a TypedDict (unavailable in Python 3.6.)

--js

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Eric Blake]

On 2/15/21 1:00 PM, John Snow wrote:
> On 2/15/21 1:25 PM, Eric Blake wrote:
>> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>> +BlockBitmapMapping = List[Dict[str,
>> +                               Union[str,
>> +                                     List[Dict[str,
>> +                                               Union[str, Dict[str,
>> bool]]]]]]]
> 
> That looks *very* beefy.
> 
> Is the Union because that union is valid for every key, or because every
> key has a potentially different value that is specific to that key?
> 
> if it's the latter, I'd ditch the Union and just go with:
> 
> Dict[str, object], or
> Dict[str, Any]
> 
> object: will allow any type, but keeps strict checking enabled. If you
> try to use that value later on without a cast, mypy will warn you if you
> are using it in a manner not guaranteed by the "object" type. Can be
> useful if you are passing values to a function that already does RTTI to
> determine behavior.

We're in luck; both 297 and 300 still pass with this applied on top of
my previous attempt:

diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
index 7501bd1018e2..adb927629747 100755
--- i/tests/qemu-iotests/300
+++ w/tests/qemu-iotests/300
@@ -22,7 +22,7 @@
 import os
 import random
 import re
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Optional

 import iotests

@@ -30,10 +30,7 @@ import iotests
 # pylint: disable=wrong-import-order
 import qemu

-BlockBitmapMapping = List[Dict[str,
-                               Union[str,
-                                     List[Dict[str,
-                                               Union[str, Dict[str,
bool]]]]]]]
+BlockBitmapMapping = List[Dict[str, object]]

 mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')



> 
> Any: Also allows any type, but enables gradual typing. If you later
> "assume" the type of this value, mypy will say nothing. Can be useful
> when you've just got a job to do and the right tool would have been a
> recursive type or a TypedDict (unavailable in Python 3.6.)

I'm not too worried about needing to further enhance the type-checking
on an individual iotest.

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

Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[John Snow]

On 2/15/21 3:26 PM, Eric Blake wrote:
> On 2/15/21 1:00 PM, John Snow wrote:
>> On 2/15/21 1:25 PM, Eric Blake wrote:
>>> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>>> +BlockBitmapMapping = List[Dict[str,
>>> +                               Union[str,
>>> +                                     List[Dict[str,
>>> +                                               Union[str, Dict[str,
>>> bool]]]]]]]
>>
>> That looks *very* beefy.
>>
>> Is the Union because that union is valid for every key, or because every
>> key has a potentially different value that is specific to that key?
>>
>> if it's the latter, I'd ditch the Union and just go with:
>>
>> Dict[str, object], or
>> Dict[str, Any]
>>
>> object: will allow any type, but keeps strict checking enabled. If you
>> try to use that value later on without a cast, mypy will warn you if you
>> are using it in a manner not guaranteed by the "object" type. Can be
>> useful if you are passing values to a function that already does RTTI to
>> determine behavior.
> 
> We're in luck; both 297 and 300 still pass with this applied on top of
> my previous attempt:
> 
> diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
> index 7501bd1018e2..adb927629747 100755
> --- i/tests/qemu-iotests/300
> +++ w/tests/qemu-iotests/300
> @@ -22,7 +22,7 @@
>   import os
>   import random
>   import re
> -from typing import Dict, List, Optional, Union
> +from typing import Dict, List, Optional
> 
>   import iotests
> 
> @@ -30,10 +30,7 @@ import iotests
>   # pylint: disable=wrong-import-order
>   import qemu
> 
> -BlockBitmapMapping = List[Dict[str,
> -                               Union[str,
> -                                     List[Dict[str,
> -                                               Union[str, Dict[str,
> bool]]]]]]]
> +BlockBitmapMapping = List[Dict[str, object]]
> 

nice :)

>   mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
> 
> 
> 
>>
>> Any: Also allows any type, but enables gradual typing. If you later
>> "assume" the type of this value, mypy will say nothing. Can be useful
>> when you've just got a job to do and the right tool would have been a
>> recursive type or a TypedDict (unavailable in Python 3.6.)
> 
> I'm not too worried about needing to further enhance the type-checking
> on an individual iotest.
> 

Yes, Agreed. I have been going very "overboard" with the python and QAPI 
types, but I consider those libraries that might have need of such 
pedantic types.

The iotests themselves? eh.

Just figured I'd give you a range of options to choose from and you'd 
pick the best one.

--js


PS: I really really really really really wish that 3.6 had TypedDict.