[PATCH v4 0/4] migration: Add block-bitmap-mapping parameter

[PATCH v4 0/4] migration: Add block-bitmap-mapping parameter

[Max Reitz]

RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html
RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html
v1: https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09792.html
v2: https://lists.nongnu.org/archive/html/qemu-block/2020-07/msg01179.html
v3: https://lists.nongnu.org/archive/html/qemu-block/2020-07/msg01385.html

Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-v4
Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-v4

Hi,

This new migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.

This way, management tools can use different node names on the source
and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).


v4:
- Patch 1:
  - Rebased
  - %s/5.1/5.2/
  - The destination side will now ignore unmapped aliases, too (though
    they are reported to stderr)
  - Check that the aliases given will fit in the migration stream (i.e.
    are shorter than 256 bytes)
    - On that note, fix a pre-existing bug where a bitmap with a name
      longer than 255 bytes would make qemu_put_counted_string() fail an
      assertion

- Patch 2:
  - import time (apparently, I forgot that in v3...?)

- Patch 3:
  - Added; I need this now

- Patch 4:
  - Migration rarely ever really fails now (just one case, where a
    non-aliased bitmap name is too long, which can only be detected when
    beginning migration)

  - Thus, we have to read most error messages from the VM log now, for
    which I’ve added a new function (verify_dest_error)

  - Added tests for overly long bitmap names and node/bitmap aliases


git-backport-diff against v3:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[0167] [FC] 'migration: Add block-bitmap-mapping parameter'
002/4:[0001] [FC] 'iotests.py: Add wait_for_runstate()'
003/4:[down] 'iotests.py: Let wait_migration() return on failure'
004/4:[0232] [FC] 'iotests: Test node/bitmap aliases during migration'


Max Reitz (4):
  migration: Add block-bitmap-mapping parameter
  iotests.py: Add wait_for_runstate()
  iotests.py: Let wait_migration() return on failure
  iotests: Test node/bitmap aliases during migration

 qapi/migration.json            | 101 +++++-
 migration/migration.h          |   3 +
 migration/block-dirty-bitmap.c | 409 +++++++++++++++++++---
 migration/migration.c          |  30 ++
 monitor/hmp-cmds.c             |  30 ++
 tests/qemu-iotests/300         | 595 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out     |   5 +
 tests/qemu-iotests/group       |   1 +
 tests/qemu-iotests/iotests.py  |  22 +-
 9 files changed, 1134 insertions(+), 62 deletions(-)
 create mode 100755 tests/qemu-iotests/300
 create mode 100644 tests/qemu-iotests/300.out

-- 
2.26.2

[PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

[Max Reitz]

This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.

This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).

While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().

Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/migration.json            | 101 +++++++-
 migration/migration.h          |   3 +
 migration/block-dirty-bitmap.c | 409 ++++++++++++++++++++++++++++-----
 migration/migration.c          |  30 +++
 monitor/hmp-cmds.c             |  30 +++
 5 files changed, 517 insertions(+), 56 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index ea53b23dca..0c4ae102b1 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -508,6 +508,44 @@
   'data': [ 'none', 'zlib',
             { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
+##
+# @BitmapMigrationBitmapAlias:
+#
+# @name: The name of the bitmap.
+#
+# @alias: An alias name for migration (for example the bitmap name on
+#         the opposite site).
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationBitmapAlias',
+  'data': {
+      'name': 'str',
+      'alias': 'str'
+  } }
+
+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+#         node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+  'data': {
+      'node-name': 'str',
+      'alias': 'str',
+      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+  } }
+
 ##
 # @MigrationParameter:
 #
@@ -642,6 +680,24 @@
 #          will consume more CPU.
 #          Defaults to 1. (Since 5.0)
 #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#          aliases for the purpose of dirty bitmap migration.  Such
+#          aliases may for example be the corresponding names on the
+#          opposite site.
+#          The mapping must be one-to-one, but not necessarily
+#          complete: On the source, unmapped bitmaps and all bitmaps
+#          on unmapped nodes will be ignored.  On the destination,
+#          all unmapped aliases in the incoming migration stream will
+#          be reported, but they will not result in failure.
+#          Note that the destination does not know about bitmaps it
+#          does not receive, so there is no limitation or requirement
+#          regarding the number of bitmaps received, or how they are
+#          named, or on which nodes they are placed.
+#          By default (when this parameter has never been set), bitmap
+#          names are mapped to themselves.  Nodes are mapped to their
+#          block device name if there is one, and to their node name
+#          otherwise. (Since 5.2)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -656,7 +712,8 @@
            'multifd-channels',
            'xbzrle-cache-size', 'max-postcopy-bandwidth',
            'max-cpu-throttle', 'multifd-compression',
-           'multifd-zlib-level' ,'multifd-zstd-level' ] }
+           'multifd-zlib-level' ,'multifd-zstd-level',
+           'block-bitmap-mapping' ] }
 
 ##
 # @MigrateSetParameters:
@@ -782,6 +839,24 @@
 #          will consume more CPU.
 #          Defaults to 1. (Since 5.0)
 #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#          aliases for the purpose of dirty bitmap migration.  Such
+#          aliases may for example be the corresponding names on the
+#          opposite site.
+#          The mapping must be one-to-one, but not necessarily
+#          complete: On the source, unmapped bitmaps and all bitmaps
+#          on unmapped nodes will be ignored.  On the destination,
+#          all unmapped aliases in the incoming migration stream will
+#          be reported, but they will not result in failure.
+#          Note that the destination does not know about bitmaps it
+#          does not receive, so there is no limitation or requirement
+#          regarding the number of bitmaps received, or how they are
+#          named, or on which nodes they are placed.
+#          By default (when this parameter has never been set), bitmap
+#          names are mapped to themselves.  Nodes are mapped to their
+#          block device name if there is one, and to their node name
+#          otherwise. (Since 5.2)
+#
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -812,7 +887,8 @@
             '*max-cpu-throttle': 'int',
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'int',
-            '*multifd-zstd-level': 'int' } }
+            '*multifd-zstd-level': 'int',
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
 # @migrate-set-parameters:
@@ -958,6 +1034,24 @@
 #          will consume more CPU.
 #          Defaults to 1. (Since 5.0)
 #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#          aliases for the purpose of dirty bitmap migration.  Such
+#          aliases may for example be the corresponding names on the
+#          opposite site.
+#          The mapping must be one-to-one, but not necessarily
+#          complete: On the source, unmapped bitmaps and all bitmaps
+#          on unmapped nodes will be ignored.  On the destination,
+#          all unmapped aliases in the incoming migration stream will
+#          be reported, but they will not result in failure.
+#          Note that the destination does not know about bitmaps it
+#          does not receive, so there is no limitation or requirement
+#          regarding the number of bitmaps received, or how they are
+#          named, or on which nodes they are placed.
+#          By default (when this parameter has never been set), bitmap
+#          names are mapped to themselves.  Nodes are mapped to their
+#          block device name if there is one, and to their node name
+#          otherwise. (Since 5.2)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -986,7 +1080,8 @@
             '*max-cpu-throttle': 'uint8',
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
-            '*multifd-zstd-level': 'uint8' } }
+            '*multifd-zstd-level': 'uint8',
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
 # @query-migrate-parameters:
diff --git a/migration/migration.h b/migration/migration.h
index 6c6a931d0d..2ed55c4aef 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -337,6 +337,9 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 void dirty_bitmap_mig_before_vm_start(void);
 void dirty_bitmap_mig_cancel_outgoing(void);
 void dirty_bitmap_mig_cancel_incoming(void);
+bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
+                                      Error **errp);
+
 void migrate_add_address(SocketAddress *address);
 
 int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 784330ebe1..89cb16b12c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -29,10 +29,10 @@
  *
  * # Header (shared for different chunk types)
  * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
- * [ 1 byte: node name size ] \  flags & DEVICE_NAME
- * [ n bytes: node name     ] /
- * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
- * [ n bytes: bitmap name     ] /
+ * [ 1 byte: node alias size ] \  flags & DEVICE_NAME
+ * [ n bytes: node alias     ] /
+ * [ 1 byte: bitmap alias size ] \  flags & BITMAP_NAME
+ * [ n bytes: bitmap alias     ] /
  *
  * # Start of bitmap migration (flags & START)
  * header
@@ -72,7 +72,9 @@
 #include "migration/register.h"
 #include "qemu/hbitmap.h"
 #include "qemu/cutils.h"
+#include "qemu/id.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
 #include "trace.h"
 
 #define CHUNK_SIZE     (1 << 10)
@@ -104,7 +106,8 @@
 typedef struct SaveBitmapState {
     /* Written during setup phase. */
     BlockDriverState *bs;
-    const char *node_name;
+    char *node_alias;
+    char *bitmap_alias;
     BdrvDirtyBitmap *bitmap;
     uint64_t total_sectors;
     uint64_t sectors_per_chunk;
@@ -138,8 +141,9 @@ typedef struct LoadBitmapState {
 /* State of the dirty bitmap migration (DBM) during load process */
 typedef struct DBMLoadState {
     uint32_t flags;
-    char node_name[256];
-    char bitmap_name[256];
+    char node_alias[256];
+    char bitmap_alias[256];
+    char bitmap_name[BDRV_BITMAP_MAX_NAME_SIZE + 1];
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
 
@@ -165,6 +169,189 @@ typedef struct DBMState {
 
 static DBMState dbm_state;
 
+/* For hash tables that map node/bitmap names to aliases */
+typedef struct AliasMapInnerNode {
+    char *string;
+    GHashTable *subtree;
+} AliasMapInnerNode;
+
+static void free_alias_map_inner_node(void *amin_ptr)
+{
+    AliasMapInnerNode *amin = amin_ptr;
+
+    g_free(amin->string);
+    g_hash_table_unref(amin->subtree);
+    g_free(amin);
+}
+
+/**
+ * Construct an alias map based on the given QMP structure.
+ *
+ * (Note that we cannot store such maps in the MigrationParameters
+ * object, because that struct is defined by the QAPI schema, which
+ * makes it basically impossible to have dicts with arbitrary keys.
+ * Therefore, we instead have to construct these maps when migration
+ * starts.)
+ *
+ * @bbm is the block_bitmap_mapping from the migration parameters.
+ *
+ * If @name_to_alias is true, the returned hash table will map node
+ * and bitmap names to their respective aliases (for outgoing
+ * migration).
+ *
+ * If @name_to_alias is false, the returned hash table will map node
+ * and bitmap aliases to their respective names (for incoming
+ * migration).
+ *
+ * The hash table maps node names/aliases to AliasMapInnerNode
+ * objects, whose .string is the respective node alias/name, and whose
+ * .subtree table maps bitmap names/aliases to the respective bitmap
+ * alias/name.
+ */
+static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
+                                       bool name_to_alias,
+                                       Error **errp)
+{
+    GHashTable *alias_map;
+    size_t max_node_name_len =
+        sizeof(((BlockDriverState *)NULL)->node_name) - 1;
+
+    alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+                                      g_free, free_alias_map_inner_node);
+
+    for (; bbm; bbm = bbm->next) {
+        const BitmapMigrationNodeAlias *bmna = bbm->value;
+        const BitmapMigrationBitmapAliasList *bmbal;
+        AliasMapInnerNode *amin;
+        GHashTable *bitmaps_map;
+        const char *node_map_from, *node_map_to;
+
+        if (!id_wellformed(bmna->alias)) {
+            error_setg(errp, "The node alias '%s' is not well-formed",
+                       bmna->alias);
+            goto fail;
+        }
+
+        if (strlen(bmna->alias) > 255) {
+            error_setg(errp, "The node alias '%s' is longer than 255 bytes",
+                       bmna->alias);
+            goto fail;
+        }
+
+        if (strlen(bmna->node_name) > max_node_name_len) {
+            error_setg(errp, "The node name '%s' is longer than %zu bytes",
+                       bmna->node_name, max_node_name_len);
+            goto fail;
+        }
+
+        if (name_to_alias) {
+            if (g_hash_table_contains(alias_map, bmna->node_name)) {
+                error_setg(errp, "The node name '%s' is mapped twice",
+                           bmna->node_name);
+                goto fail;
+            }
+
+            node_map_from = bmna->node_name;
+            node_map_to = bmna->alias;
+        } else {
+            if (g_hash_table_contains(alias_map, bmna->alias)) {
+                error_setg(errp, "The node alias '%s' is used twice",
+                           bmna->alias);
+                goto fail;
+            }
+
+            node_map_from = bmna->alias;
+            node_map_to = bmna->node_name;
+        }
+
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+                                            g_free, g_free);
+
+        amin = g_new(AliasMapInnerNode, 1);
+        *amin = (AliasMapInnerNode){
+            .string = g_strdup(node_map_to),
+            .subtree = bitmaps_map,
+        };
+
+        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
+
+        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+            const char *bmap_map_from, *bmap_map_to;
+
+            if (strlen(bmba->alias) > 255) {
+                error_setg(errp,
+                           "The bitmap alias '%s' is longer than 255 bytes",
+                           bmba->alias);
+                goto fail;
+            }
+
+            if (strlen(bmba->name) > BDRV_BITMAP_MAX_NAME_SIZE) {
+                error_setg(errp, "The bitmap name '%s' is longer than %d bytes",
+                           bmba->name, BDRV_BITMAP_MAX_NAME_SIZE);
+                goto fail;
+            }
+
+            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",
+                               bmna->node_name, bmba->name);
+                    goto fail;
+                }
+            } 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",
+                               bmna->alias, bmba->alias);
+                    goto fail;
+                }
+            }
+
+            g_hash_table_insert(bitmaps_map,
+                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
+        }
+    }
+
+    return alias_map;
+
+fail:
+    g_hash_table_destroy(alias_map);
+    return NULL;
+}
+
+/**
+ * Run construct_alias_map() in both directions to check whether @bbm
+ * is valid.
+ * (This function is to be used by migration/migration.c to validate
+ * the user-specified block-bitmap-mapping migration parameter.)
+ *
+ * Returns true if and only if the mapping is valid.
+ */
+bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
+                                      Error **errp)
+{
+    GHashTable *alias_map;
+
+    alias_map = construct_alias_map(bbm, true, errp);
+    if (!alias_map) {
+        return false;
+    }
+    g_hash_table_destroy(alias_map);
+
+    alias_map = construct_alias_map(bbm, false, errp);
+    if (!alias_map) {
+        return false;
+    }
+    g_hash_table_destroy(alias_map);
+
+    return true;
+}
+
 static uint32_t qemu_get_bitmap_flags(QEMUFile *f)
 {
     uint8_t flags = qemu_get_byte(f);
@@ -207,11 +394,11 @@ static void send_bitmap_header(QEMUFile *f, DBMSaveState *s,
     qemu_put_bitmap_flags(f, flags);
 
     if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
-        qemu_put_counted_string(f, dbms->node_name);
+        qemu_put_counted_string(f, dbms->node_alias);
     }
 
     if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
-        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
+        qemu_put_counted_string(f, dbms->bitmap_alias);
     }
 }
 
@@ -282,18 +469,25 @@ static void dirty_bitmap_do_save_cleanup(DBMSaveState *s)
         QSIMPLEQ_REMOVE_HEAD(&s->dbms_list, entry);
         bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
         bdrv_unref(dbms->bs);
+        g_free(dbms->node_alias);
+        g_free(dbms->bitmap_alias);
         g_free(dbms);
     }
 }
 
 /* Called with iothread lock taken. */
 static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
-                               const char *bs_name)
+                               const char *bs_name, GHashTable *alias_map)
 {
     BdrvDirtyBitmap *bitmap;
     SaveBitmapState *dbms;
+    GHashTable *bitmap_aliases;
+    const char *node_alias, *bitmap_name, *bitmap_alias;
     Error *local_err = NULL;
 
+    /* When an alias map is given, @bs_name must be @bs's node name */
+    assert(!alias_map || !strcmp(bs_name, bdrv_get_node_name(bs)));
+
     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
         if (bdrv_dirty_bitmap_name(bitmap)) {
             break;
@@ -303,21 +497,39 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
         return 0;
     }
 
+    bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+
     if (!bs_name || strcmp(bs_name, "") == 0) {
         error_report("Bitmap '%s' in unnamed node can't be migrated",
-                     bdrv_dirty_bitmap_name(bitmap));
+                     bitmap_name);
         return -1;
     }
 
-    if (bs_name[0] == '#') {
+    if (alias_map) {
+        const AliasMapInnerNode *amin = g_hash_table_lookup(alias_map, bs_name);
+
+        if (!amin) {
+            /* Skip bitmaps on nodes with no alias */
+            return 0;
+        }
+
+        node_alias = amin->string;
+        bitmap_aliases = amin->subtree;
+    } else {
+        node_alias = bs_name;
+        bitmap_aliases = NULL;
+    }
+
+    if (node_alias[0] == '#') {
         error_report("Bitmap '%s' in a node with auto-generated "
                      "name '%s' can't be migrated",
-                     bdrv_dirty_bitmap_name(bitmap), bs_name);
+                     bitmap_name, node_alias);
         return -1;
     }
 
     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
-        if (!bdrv_dirty_bitmap_name(bitmap)) {
+        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+        if (!bitmap_name) {
             continue;
         }
 
@@ -326,12 +538,29 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
             return -1;
         }
 
+        if (bitmap_aliases) {
+            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+            if (!bitmap_alias) {
+                /* Skip bitmaps with no alias */
+                continue;
+            }
+        } else {
+            if (strlen(bitmap_name) > 255) {
+                error_report("Cannot migrate bitmap '%s' on node '%s': "
+                             "Name is longer than 255 bytes",
+                             bitmap_name, bs_name);
+                return -1;
+            }
+            bitmap_alias = bitmap_name;
+        }
+
         bdrv_ref(bs);
         bdrv_dirty_bitmap_set_busy(bitmap, true);
 
         dbms = g_new0(SaveBitmapState, 1);
         dbms->bs = bs;
-        dbms->node_name = bs_name;
+        dbms->node_alias = g_strdup(node_alias);
+        dbms->bitmap_alias = g_strdup(bitmap_alias);
         dbms->bitmap = bitmap;
         dbms->total_sectors = bdrv_nb_sectors(bs);
         dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
@@ -356,43 +585,52 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
     SaveBitmapState *dbms;
     GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
     BlockBackend *blk;
+    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
+    GHashTable *alias_map = NULL;
+
+    if (mig_params->has_block_bitmap_mapping) {
+        alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
+                                        &error_abort);
+    }
 
     s->bulk_completed = false;
     s->prev_bs = NULL;
     s->prev_bitmap = NULL;
     s->no_bitmaps = false;
 
-    /*
-     * Use blockdevice name for direct (or filtered) children of named block
-     * backends.
-     */
-    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
-        const char *name = blk_name(blk);
-
-        if (!name || strcmp(name, "") == 0) {
-            continue;
-        }
+    if (!alias_map) {
+        /*
+         * Use blockdevice name for direct (or filtered) children of named block
+         * backends.
+         */
+        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+            const char *name = blk_name(blk);
 
-        bs = blk_bs(blk);
+            if (!name || strcmp(name, "") == 0) {
+                continue;
+            }
 
-        /* Skip filters without bitmaps */
-        while (bs && bs->drv && bs->drv->is_filter &&
-               !bdrv_has_named_bitmaps(bs))
-        {
-            if (bs->backing) {
-                bs = bs->backing->bs;
-            } else if (bs->file) {
-                bs = bs->file->bs;
-            } else {
-                bs = NULL;
+            bs = blk_bs(blk);
+
+            /* Skip filters without bitmaps */
+            while (bs && bs->drv && bs->drv->is_filter &&
+                   !bdrv_has_named_bitmaps(bs))
+            {
+                if (bs->backing) {
+                    bs = bs->backing->bs;
+                } else if (bs->file) {
+                    bs = bs->file->bs;
+                } else {
+                    bs = NULL;
+                }
             }
-        }
 
-        if (bs && bs->drv && !bs->drv->is_filter) {
-            if (add_bitmaps_to_list(s, bs, name)) {
-                goto fail;
+            if (bs && bs->drv && !bs->drv->is_filter) {
+                if (add_bitmaps_to_list(s, bs, name, NULL)) {
+                    goto fail;
+                }
+                g_hash_table_add(handled_by_blk, bs);
             }
-            g_hash_table_add(handled_by_blk, bs);
         }
     }
 
@@ -401,7 +639,7 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
             continue;
         }
 
-        if (add_bitmaps_to_list(s, bs, bdrv_get_node_name(bs))) {
+        if (add_bitmaps_to_list(s, bs, bdrv_get_node_name(bs), alias_map)) {
             goto fail;
         }
     }
@@ -416,11 +654,17 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
     }
 
     g_hash_table_destroy(handled_by_blk);
+    if (alias_map) {
+        g_hash_table_destroy(alias_map);
+    }
 
     return 0;
 
 fail:
     g_hash_table_destroy(handled_by_blk);
+    if (alias_map) {
+        g_hash_table_destroy(alias_map);
+    }
     dirty_bitmap_do_save_cleanup(s);
 
     return -1;
@@ -770,8 +1014,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
     return 0;
 }
 
-static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
+static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
+                                    GHashTable *alias_map)
 {
+    GHashTable *bitmap_alias_map = NULL;
     Error *local_err = NULL;
     bool nothing;
     s->flags = qemu_get_bitmap_flags(f);
@@ -780,28 +1026,73 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
     nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
 
     if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
-        if (!qemu_get_counted_string(f, s->node_name)) {
-            error_report("Unable to read node name string");
+        if (!qemu_get_counted_string(f, s->node_alias)) {
+            error_report("Unable to read node alias string");
             return -EINVAL;
         }
+
         if (!s->cancelled) {
-            s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
+            if (alias_map) {
+                const AliasMapInnerNode *amin;
+
+                amin = g_hash_table_lookup(alias_map, s->node_alias);
+                if (!amin) {
+                    error_setg(&local_err, "Error: Unknown node alias '%s'",
+                               s->node_alias);
+                    s->bs = NULL;
+                } else {
+                    bitmap_alias_map = amin->subtree;
+                    s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
+                }
+            } else {
+                s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias,
+                                       &local_err);
+            }
             if (!s->bs) {
                 error_report_err(local_err);
                 cancel_incoming_locked(s);
             }
         }
-    } else if (!s->bs && !nothing && !s->cancelled) {
+    } else if (s->bs) {
+        if (alias_map) {
+            const AliasMapInnerNode *amin;
+
+            /* Must be present in the map, or s->bs would not be set */
+            amin = g_hash_table_lookup(alias_map, s->node_alias);
+            assert(amin != NULL);
+
+            bitmap_alias_map = amin->subtree;
+        }
+    } else if (!nothing && !s->cancelled) {
         error_report("Error: block device name is not set");
         cancel_incoming_locked(s);
     }
 
+    assert(nothing || s->cancelled || !!alias_map == !!bitmap_alias_map);
+
     if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
-        if (!qemu_get_counted_string(f, s->bitmap_name)) {
+        const char *bitmap_name;
+
+        if (!qemu_get_counted_string(f, s->bitmap_alias)) {
             error_report("Unable to read bitmap name string");
             return -EINVAL;
         }
+
         if (!s->cancelled) {
+            if (bitmap_alias_map) {
+                bitmap_name = g_hash_table_lookup(bitmap_alias_map,
+                                                  s->bitmap_alias);
+                if (!bitmap_name) {
+                    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 = s->bitmap_alias;
+            }
+
+            g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
             s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
 
             /*
@@ -811,7 +1102,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
             if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
                 error_report("Error: unknown dirty bitmap "
                              "'%s' for block device '%s'",
-                             s->bitmap_name, s->node_name);
+                             s->bitmap_name, s->bs->node_name);
                 cancel_incoming_locked(s);
             }
         }
@@ -835,6 +1126,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
  */
 static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
 {
+    GHashTable *alias_map = NULL;
+    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
     DBMLoadState *s = &((DBMState *)opaque)->load;
     int ret = 0;
 
@@ -846,13 +1139,18 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
         return -EINVAL;
     }
 
+    if (mig_params->has_block_bitmap_mapping) {
+        alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
+                                        false, &error_abort);
+    }
+
     do {
         QEMU_LOCK_GUARD(&s->lock);
 
-        ret = dirty_bitmap_load_header(f, s);
+        ret = dirty_bitmap_load_header(f, s, alias_map);
         if (ret < 0) {
             cancel_incoming_locked(s);
-            return ret;
+            goto fail;
         }
 
         if (s->flags & DIRTY_BITMAP_MIG_FLAG_START) {
@@ -869,12 +1167,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
 
         if (ret) {
             cancel_incoming_locked(s);
-            return ret;
+            goto fail;
         }
     } while (!(s->flags & DIRTY_BITMAP_MIG_FLAG_EOS));
 
     trace_dirty_bitmap_load_success();
-    return 0;
+    ret = 0;
+fail:
+    if (alias_map) {
+        g_hash_table_destroy(alias_map);
+    }
+    return ret;
 }
 
 static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
diff --git a/migration/migration.c b/migration/migration.c
index 8fe36339db..dbd4afa1e8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -36,6 +36,7 @@
 #include "block/block.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-migration.h"
 #include "qapi/qapi-visit-sockets.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-events-migration.h"
@@ -843,6 +844,13 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->has_announce_step = true;
     params->announce_step = s->parameters.announce_step;
 
+    if (s->parameters.has_block_bitmap_mapping) {
+        params->has_block_bitmap_mapping = true;
+        params->block_bitmap_mapping =
+            QAPI_CLONE(BitmapMigrationNodeAliasList,
+                       s->parameters.block_bitmap_mapping);
+    }
+
     return params;
 }
 
@@ -1308,6 +1316,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
                    "is invalid, it must be in the range of 1 to 10000 ms");
        return false;
     }
+
+    if (params->has_block_bitmap_mapping &&
+        !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) {
+        error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
+        return false;
+    }
+
     return true;
 }
 
@@ -1402,6 +1417,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_announce_step) {
         dest->announce_step = params->announce_step;
     }
+
+    if (params->has_block_bitmap_mapping) {
+        dest->has_block_bitmap_mapping = true;
+        dest->block_bitmap_mapping = params->block_bitmap_mapping;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1514,6 +1534,16 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_announce_step) {
         s->parameters.announce_step = params->announce_step;
     }
+
+    if (params->has_block_bitmap_mapping) {
+        qapi_free_BitmapMigrationNodeAliasList(
+            s->parameters.block_bitmap_mapping);
+
+        s->parameters.has_block_bitmap_mapping = true;
+        s->parameters.block_bitmap_mapping =
+            QAPI_CLONE(BitmapMigrationNodeAliasList,
+                       params->block_bitmap_mapping);
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ae4b6a4246..7711726fd2 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -469,6 +469,32 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: '%s'\n",
             MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
             params->tls_authz);
+
+        if (params->has_block_bitmap_mapping) {
+            const BitmapMigrationNodeAliasList *bmnal;
+
+            monitor_printf(mon, "%s:\n",
+                           MigrationParameter_str(
+                               MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
+
+            for (bmnal = params->block_bitmap_mapping;
+                 bmnal;
+                 bmnal = bmnal->next)
+            {
+                const BitmapMigrationNodeAlias *bmna = bmnal->value;
+                const BitmapMigrationBitmapAliasList *bmbal;
+
+                monitor_printf(mon, "  '%s' -> '%s'\n",
+                               bmna->node_name, bmna->alias);
+
+                for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+                    const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+
+                    monitor_printf(mon, "    '%s' -> '%s'\n",
+                                   bmba->name, bmba->alias);
+                }
+            }
+        }
     }
 
     qapi_free_MigrationParameters(params);
@@ -1384,6 +1410,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_announce_step = true;
         visit_type_size(v, param, &p->announce_step, &err);
         break;
+    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
+        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
+                   "through QMP");
+        break;
     default:
         assert(0);
     }
-- 
2.26.2

Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

[Eric Blake]

On 8/18/20 8:32 AM, Max Reitz wrote:
> This migration parameter allows mapping block node names and bitmap
> names to aliases for the purpose of block dirty bitmap migration.
> 
> This way, management tools can use different node and bitmap names on
> the source and destination and pass the mapping of how bitmaps are to be
> transferred to qemu (on the source, the destination, or even both with
> arbitrary aliases in the migration stream).
> 
> While touching this code, fix a bug where bitmap names longer than 255
> bytes would fail an assertion in qemu_put_counted_string().
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> +##
> +# @BitmapMigrationNodeAlias:
> +#
> +# Maps a block node name and the bitmaps it has to aliases for dirty
> +# bitmap migration.
> +#
> +# @node-name: A block node name.
> +#
> +# @alias: An alias block node name for migration (for example the
> +#         node name on the opposite site).
> +#
> +# @bitmaps: Mappings for the bitmaps on this node.
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'BitmapMigrationNodeAlias',
> +  'data': {
> +      'node-name': 'str',
> +      'alias': 'str',
> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> +  } }

Possible change: should 'alias' be optional (if absent, it defaults to 
'node-name')?  But that can be done on top, if we like it.


> +static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                       bool name_to_alias,
> +                                       Error **errp)
> +{
> +    GHashTable *alias_map;
> +    size_t max_node_name_len =
> +        sizeof(((BlockDriverState *)NULL)->node_name) - 1;

Looks a bit nicer as = sizeof_field(BlockDriverState, node_name) - 1.

> +
> +    alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> +                                      g_free, free_alias_map_inner_node);
> +
> +    for (; bbm; bbm = bbm->next) {
> +        const BitmapMigrationNodeAlias *bmna = bbm->value;
> +        const BitmapMigrationBitmapAliasList *bmbal;
> +        AliasMapInnerNode *amin;
> +        GHashTable *bitmaps_map;
> +        const char *node_map_from, *node_map_to;
> +
> +        if (!id_wellformed(bmna->alias)) {
> +            error_setg(errp, "The node alias '%s' is not well-formed",
> +                       bmna->alias);
> +            goto fail;
> +        }
> +
> +        if (strlen(bmna->alias) > 255) {

Magic number.  UINT8_MAX seems better (since the limit really is due to 
our migration format limiting to one byte).

...
> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
> +
> +        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> +            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> +            const char *bmap_map_from, *bmap_map_to;
> +
> +            if (strlen(bmba->alias) > 255) {

and again

> +                error_setg(errp,
> +                           "The bitmap alias '%s' is longer than 255 bytes",
> +                           bmba->alias);
> +                goto fail;
> +            }

Thanks for adding in the length checking since last revision!


> @@ -326,12 +538,29 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
>               return -1;
>           }
>   
> +        if (bitmap_aliases) {
> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
> +            if (!bitmap_alias) {
> +                /* Skip bitmaps with no alias */
> +                continue;
> +            }
> +        } else {
> +            if (strlen(bitmap_name) > 255) {
> +                error_report("Cannot migrate bitmap '%s' on node '%s': "
> +                             "Name is longer than 255 bytes",
> +                             bitmap_name, bs_name);
> +                return -1;

Another one.


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

I'm happy to make those touchups, and put this on my bitmaps queue for a 
pull request as soon as Paolo's meson stuff stabilizes.


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

Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5251 bytes --]

On 20.08.20 03:17, Eric Blake wrote:
> On 8/18/20 8:32 AM, Max Reitz wrote:
>> This migration parameter allows mapping block node names and bitmap
>> names to aliases for the purpose of block dirty bitmap migration.
>>
>> This way, management tools can use different node and bitmap names on
>> the source and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> While touching this code, fix a bug where bitmap names longer than 255
>> bytes would fail an assertion in qemu_put_counted_string().
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
>> +##
>> +# @BitmapMigrationNodeAlias:
>> +#
>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>> +# bitmap migration.
>> +#
>> +# @node-name: A block node name.
>> +#
>> +# @alias: An alias block node name for migration (for example the
>> +#         node name on the opposite site).
>> +#
>> +# @bitmaps: Mappings for the bitmaps on this node.
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'struct': 'BitmapMigrationNodeAlias',
>> +  'data': {
>> +      'node-name': 'str',
>> +      'alias': 'str',
>> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
>> +  } }
> 
> Possible change: should 'alias' be optional (if absent, it defaults to
> 'node-name')?  But that can be done on top, if we like it.

I don’t know whether anyone would make use of that, though, so I’d defer
it until someone wants it.

>> +static GHashTable *construct_alias_map(const
>> BitmapMigrationNodeAliasList *bbm,
>> +                                       bool name_to_alias,
>> +                                       Error **errp)
>> +{
>> +    GHashTable *alias_map;
>> +    size_t max_node_name_len =
>> +        sizeof(((BlockDriverState *)NULL)->node_name) - 1;
> 
> Looks a bit nicer as = sizeof_field(BlockDriverState, node_name) - 1.

Oh, I didn’t know we had that.

>> +
>> +    alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
>> +                                      g_free,
>> free_alias_map_inner_node);
>> +
>> +    for (; bbm; bbm = bbm->next) {
>> +        const BitmapMigrationNodeAlias *bmna = bbm->value;
>> +        const BitmapMigrationBitmapAliasList *bmbal;
>> +        AliasMapInnerNode *amin;
>> +        GHashTable *bitmaps_map;
>> +        const char *node_map_from, *node_map_to;
>> +
>> +        if (!id_wellformed(bmna->alias)) {
>> +            error_setg(errp, "The node alias '%s' is not well-formed",
>> +                       bmna->alias);
>> +            goto fail;
>> +        }
>> +
>> +        if (strlen(bmna->alias) > 255) {
> 
> Magic number.  UINT8_MAX seems better (since the limit really is due to
> our migration format limiting to one byte).

Well, yes, but qemu_put_counted_string() uses that magic number, too.
*shrug*

>> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
>> +
>> +        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
>> +            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
>> +            const char *bmap_map_from, *bmap_map_to;
>> +
>> +            if (strlen(bmba->alias) > 255) {
> 
> and again
> 
>> +                error_setg(errp,
>> +                           "The bitmap alias '%s' is longer than 255
>> bytes",
>> +                           bmba->alias);
>> +                goto fail;
>> +            }
> 
> Thanks for adding in the length checking since last revision!
> 
> 
>> @@ -326,12 +538,29 @@ static int add_bitmaps_to_list(DBMSaveState *s,
>> BlockDriverState *bs,
>>               return -1;
>>           }
>>   +        if (bitmap_aliases) {
>> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases,
>> bitmap_name);
>> +            if (!bitmap_alias) {
>> +                /* Skip bitmaps with no alias */
>> +                continue;
>> +            }
>> +        } else {
>> +            if (strlen(bitmap_name) > 255) {
>> +                error_report("Cannot migrate bitmap '%s' on node '%s': "
>> +                             "Name is longer than 255 bytes",
>> +                             bitmap_name, bs_name);
>> +                return -1;
> 
> Another one.
> 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I'm happy to make those touchups, and put this on my bitmaps queue for a
> pull request as soon as Paolo's meson stuff stabilizes.

Sounds good to me, thanks!

Max


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

Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

[Vladimir Sementsov-Ogievskiy]

18.08.2020 16:32, Max Reitz wrote:
> This migration parameter allows mapping block node names and bitmap
> names to aliases for the purpose of block dirty bitmap migration.
> 
> This way, management tools can use different node and bitmap names on
> the source and destination and pass the mapping of how bitmaps are to be
> transferred to qemu (on the source, the destination, or even both with
> arbitrary aliases in the migration stream).
> 
> While touching this code, fix a bug where bitmap names longer than 255
> bytes would fail an assertion in qemu_put_counted_string().
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qapi/migration.json            | 101 +++++++-
>   migration/migration.h          |   3 +
>   migration/block-dirty-bitmap.c | 409 ++++++++++++++++++++++++++++-----
>   migration/migration.c          |  30 +++
>   monitor/hmp-cmds.c             |  30 +++
>   5 files changed, 517 insertions(+), 56 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index ea53b23dca..0c4ae102b1 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json

[..]

>   #
> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> +#          aliases for the purpose of dirty bitmap migration.  Such
> +#          aliases may for example be the corresponding names on the
> +#          opposite site.
> +#          The mapping must be one-to-one, but not necessarily
> +#          complete: On the source, unmapped bitmaps and all bitmaps
> +#          on unmapped nodes will be ignored.  On the destination,
> +#          all unmapped aliases in the incoming migration stream will
> +#          be reported, but they will not result in failure.
Actually, on unknown alias we cancel incoming bitmap migration, which means that destination vm continues to run, other (non-bitmap) migration states continue to migrate but all further chunks of bitmap migration will be ignored. (I'm not sure it worth be mentioned)

> +#          Note that the destination does not know about bitmaps it
> +#          does not receive, so there is no limitation or requirement
> +#          regarding the number of bitmaps received, or how they are
> +#          named, or on which nodes they are placed.
> +#          By default (when this parameter has never been set), bitmap
> +#          names are mapped to themselves.  Nodes are mapped to their
> +#          block device name if there is one, and to their node name
> +#          otherwise. (Since 5.2)
> +#
>   # Since: 2.4
>   ##
>   { 'enum': 'MigrationParameter',
> @@ -656,7 +712,8 @@
>              'multifd-channels',
>              'xbzrle-cache-size', 'max-postcopy-bandwidth',
>              'max-cpu-throttle', 'multifd-compression',
> -           'multifd-zlib-level' ,'multifd-zstd-level' ] }
> +           'multifd-zlib-level' ,'multifd-zstd-level',
> +           'block-bitmap-mapping' ] }
>   
>   ##

[..]

> @@ -303,21 +497,39 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
>           return 0;
>       }
>   
> +    bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> +
>       if (!bs_name || strcmp(bs_name, "") == 0) {
>           error_report("Bitmap '%s' in unnamed node can't be migrated",
> -                     bdrv_dirty_bitmap_name(bitmap));
> +                     bitmap_name);
>           return -1;
>       }
>   
> -    if (bs_name[0] == '#') {
> +    if (alias_map) {
> +        const AliasMapInnerNode *amin = g_hash_table_lookup(alias_map, bs_name);
> +
> +        if (!amin) {
> +            /* Skip bitmaps on nodes with no alias */
> +            return 0;
> +        }
> +
> +        node_alias = amin->string;
> +        bitmap_aliases = amin->subtree;
> +    } else {
> +        node_alias = bs_name;
> +        bitmap_aliases = NULL;
> +    }
> +
> +    if (node_alias[0] == '#') {
>           error_report("Bitmap '%s' in a node with auto-generated "
>                        "name '%s' can't be migrated",
> -                     bdrv_dirty_bitmap_name(bitmap), bs_name);
> +                     bitmap_name, node_alias);
>           return -1;
>       }

This check is related only to pre-alias_map behavior, so it's probably better to keep it inside else{} branch above. Still, aliases already checked to be wellformed, so this check will be always false anyway for aliases and will not hurt.

>   
>       FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> -        if (!bdrv_dirty_bitmap_name(bitmap)) {
> +        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> +        if (!bitmap_name) {
>               continue;
>           }
>   

[..]

> @@ -770,8 +1014,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
>       return 0;
>   }
>   
> -static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
> +static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
> +                                    GHashTable *alias_map)
>   {
> +    GHashTable *bitmap_alias_map = NULL;
>       Error *local_err = NULL;
>       bool nothing;
>       s->flags = qemu_get_bitmap_flags(f);
> @@ -780,28 +1026,73 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
>       nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
>   
>       if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> -        if (!qemu_get_counted_string(f, s->node_name)) {
> -            error_report("Unable to read node name string");
> +        if (!qemu_get_counted_string(f, s->node_alias)) {
> +            error_report("Unable to read node alias string");
>               return -EINVAL;
>           }
> +
>           if (!s->cancelled) {
> -            s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> +            if (alias_map) {
> +                const AliasMapInnerNode *amin;
> +
> +                amin = g_hash_table_lookup(alias_map, s->node_alias);
> +                if (!amin) {
> +                    error_setg(&local_err, "Error: Unknown node alias '%s'",
> +                               s->node_alias);
> +                    s->bs = NULL;
> +                } else {
> +                    bitmap_alias_map = amin->subtree;
> +                    s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
> +                }
> +            } else {
> +                s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias,
> +                                       &local_err);
> +            }
>               if (!s->bs) {
>                   error_report_err(local_err);
>                   cancel_incoming_locked(s);
>               }
>           }
> -    } else if (!s->bs && !nothing && !s->cancelled) {
> +    } else if (s->bs) {
> +        if (alias_map) {
> +            const AliasMapInnerNode *amin;
> +
> +            /* Must be present in the map, or s->bs would not be set */
> +            amin = g_hash_table_lookup(alias_map, s->node_alias);
> +            assert(amin != NULL);
> +
> +            bitmap_alias_map = amin->subtree;
> +        }
> +    } else if (!nothing && !s->cancelled) {
>           error_report("Error: block device name is not set");
>           cancel_incoming_locked(s);
>       }
>   
> +    assert(nothing || s->cancelled || !!alias_map == !!bitmap_alias_map);
> +
>       if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> -        if (!qemu_get_counted_string(f, s->bitmap_name)) {
> +        const char *bitmap_name;
> +
> +        if (!qemu_get_counted_string(f, s->bitmap_alias)) {
>               error_report("Unable to read bitmap name string");

Probably s/name/alias/ like for node error message.

>               return -EINVAL;
>           }
> +
>           if (!s->cancelled) {
> +            if (bitmap_alias_map) {
> +                bitmap_name = g_hash_table_lookup(bitmap_alias_map,
> +                                                  s->bitmap_alias);
> +                if (!bitmap_name) {
> +                    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 = s->bitmap_alias;
> +            }
> +
> +            g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>               s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>   
>               /*

[..]

> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -469,6 +469,32 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>           monitor_printf(mon, "%s: '%s'\n",
>               MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
>               params->tls_authz);
> +
> +        if (params->has_block_bitmap_mapping) {
> +            const BitmapMigrationNodeAliasList *bmnal;
> +
> +            monitor_printf(mon, "%s:\n",
> +                           MigrationParameter_str(
> +                               MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
> +
> +            for (bmnal = params->block_bitmap_mapping;
> +                 bmnal;
> +                 bmnal = bmnal->next)
> +            {
> +                const BitmapMigrationNodeAlias *bmna = bmnal->value;
> +                const BitmapMigrationBitmapAliasList *bmbal;
> +
> +                monitor_printf(mon, "  '%s' -> '%s'\n",

'->' would look strange for incoming. Maybe, change to '--' or '~'.

> +                               bmna->node_name, bmna->alias);
> +
> +                for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> +                    const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> +
> +                    monitor_printf(mon, "    '%s' -> '%s'\n",
> +                                   bmba->name, bmba->alias);
> +                }
> +            }
> +        }
>       }
>   
>       qapi_free_MigrationParameters(params);
> @@ -1384,6 +1410,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>           p->has_announce_step = true;
>           visit_type_size(v, param, &p->announce_step, &err);
>           break;
> +    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
> +        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
> +                   "through QMP");
> +        break;
>       default:
>           assert(0);
>       }
> 

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

-- 
Best regards,
Vladimir

Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 6192 bytes --]

On 20.08.20 14:58, Vladimir Sementsov-Ogievskiy wrote:
> 18.08.2020 16:32, Max Reitz wrote:
>> This migration parameter allows mapping block node names and bitmap
>> names to aliases for the purpose of block dirty bitmap migration.
>>
>> This way, management tools can use different node and bitmap names on
>> the source and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> While touching this code, fix a bug where bitmap names longer than 255
>> bytes would fail an assertion in qemu_put_counted_string().
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qapi/migration.json            | 101 +++++++-
>>   migration/migration.h          |   3 +
>>   migration/block-dirty-bitmap.c | 409 ++++++++++++++++++++++++++++-----
>>   migration/migration.c          |  30 +++
>>   monitor/hmp-cmds.c             |  30 +++
>>   5 files changed, 517 insertions(+), 56 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index ea53b23dca..0c4ae102b1 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
> 
> [..]
> 
>>   #
>> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>> +#          aliases for the purpose of dirty bitmap migration.  Such
>> +#          aliases may for example be the corresponding names on the
>> +#          opposite site.
>> +#          The mapping must be one-to-one, but not necessarily
>> +#          complete: On the source, unmapped bitmaps and all bitmaps
>> +#          on unmapped nodes will be ignored.  On the destination,
>> +#          all unmapped aliases in the incoming migration stream will
>> +#          be reported, but they will not result in failure.
> Actually, on unknown alias we cancel incoming bitmap migration, which
> means that destination vm continues to run, other (non-bitmap) migration
> states continue to migrate but all further chunks of bitmap migration
> will be ignored. (I'm not sure it worth be mentioned)

Ah, yeah.

[...]

>> @@ -303,21 +497,39 @@ static int add_bitmaps_to_list(DBMSaveState *s,
>> BlockDriverState *bs,
>>           return 0;
>>       }
>>   +    bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>> +
>>       if (!bs_name || strcmp(bs_name, "") == 0) {
>>           error_report("Bitmap '%s' in unnamed node can't be migrated",
>> -                     bdrv_dirty_bitmap_name(bitmap));
>> +                     bitmap_name);
>>           return -1;
>>       }
>>   -    if (bs_name[0] == '#') {
>> +    if (alias_map) {
>> +        const AliasMapInnerNode *amin =
>> g_hash_table_lookup(alias_map, bs_name);
>> +
>> +        if (!amin) {
>> +            /* Skip bitmaps on nodes with no alias */
>> +            return 0;
>> +        }
>> +
>> +        node_alias = amin->string;
>> +        bitmap_aliases = amin->subtree;
>> +    } else {
>> +        node_alias = bs_name;
>> +        bitmap_aliases = NULL;
>> +    }
>> +
>> +    if (node_alias[0] == '#') {
>>           error_report("Bitmap '%s' in a node with auto-generated "
>>                        "name '%s' can't be migrated",
>> -                     bdrv_dirty_bitmap_name(bitmap), bs_name);
>> +                     bitmap_name, node_alias);
>>           return -1;
>>       }
> 
> This check is related only to pre-alias_map behavior, so it's probably
> better to keep it inside else{} branch above. Still, aliases already
> checked to be wellformed, so this check will be always false anyway for
> aliases and will not hurt.

Hm, it’s a trade off.  It does look a bit weird, because how can aliases
be auto-generated?  But OTOH it makes it clearer that we’ll never allow
non-wellformed aliases through.

[...]

>>       if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>> -        if (!qemu_get_counted_string(f, s->bitmap_name)) {
>> +        const char *bitmap_name;
>> +
>> +        if (!qemu_get_counted_string(f, s->bitmap_alias)) {
>>               error_report("Unable to read bitmap name string");
> 
> Probably s/name/alias/ like for node error message.

Why not.

[...]

>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -469,6 +469,32 @@ void hmp_info_migrate_parameters(Monitor *mon,
>> const QDict *qdict)
>>           monitor_printf(mon, "%s: '%s'\n",
>>               MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
>>               params->tls_authz);
>> +
>> +        if (params->has_block_bitmap_mapping) {
>> +            const BitmapMigrationNodeAliasList *bmnal;
>> +
>> +            monitor_printf(mon, "%s:\n",
>> +                           MigrationParameter_str(
>> +                              
>> MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
>> +
>> +            for (bmnal = params->block_bitmap_mapping;
>> +                 bmnal;
>> +                 bmnal = bmnal->next)
>> +            {
>> +                const BitmapMigrationNodeAlias *bmna = bmnal->value;
>> +                const BitmapMigrationBitmapAliasList *bmbal;
>> +
>> +                monitor_printf(mon, "  '%s' -> '%s'\n",
> 
> '->' would look strange for incoming. Maybe, change to '--' or '~'.

Hmm, I prefer ->.  The object’s name is the node/bitmap name, and that
object gets an alias.  So I find this to make sense even on the incoming
side.

Max


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

[PATCH v4 2/4] iotests.py: Add wait_for_runstate()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 717b5b652c..ee93cf22db 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -833,6 +833,10 @@ class VM(qtest.QEMUQtestMachine):
                    'Found node %s under %s (but expected %s)' % \
                    (node['name'], path, expected_node)
 
+    def wait_for_runstate(self, runstate: str) -> None:
+        while self.qmp('query-status')['return']['status'] != runstate:
+            time.sleep(0.2)
+
 index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
 
 class QMPTestCase(unittest.TestCase):
-- 
2.26.2

Re: [PATCH v4 2/4] iotests.py: Add wait_for_runstate()

[Eric Blake]

On 8/18/20 8:32 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 4 ++++
>   1 file changed, 4 insertions(+)

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

> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 717b5b652c..ee93cf22db 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -833,6 +833,10 @@ class VM(qtest.QEMUQtestMachine):
>                      'Found node %s under %s (but expected %s)' % \
>                      (node['name'], path, expected_node)
>   
> +    def wait_for_runstate(self, runstate: str) -> None:
> +        while self.qmp('query-status')['return']['status'] != runstate:
> +            time.sleep(0.2)

This looks like it could inf-loop if we have a bug where the status 
never changes as expected; but I guess CI bots have timeouts at higher 
layers that would detect if such a bug sneaks in.

> +
>   index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
>   
>   class QMPTestCase(unittest.TestCase):
> 

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

Re: [PATCH v4 2/4] iotests.py: Add wait_for_runstate()

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 8/18/20 8:32 AM, Max Reitz wrote:
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py | 4 ++++
> >   1 file changed, 4 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 717b5b652c..ee93cf22db 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -833,6 +833,10 @@ class VM(qtest.QEMUQtestMachine):
> >                      'Found node %s under %s (but expected %s)' % \
> >                      (node['name'], path, expected_node)
> > +    def wait_for_runstate(self, runstate: str) -> None:
> > +        while self.qmp('query-status')['return']['status'] != runstate:
> > +            time.sleep(0.2)
> 
> This looks like it could inf-loop if we have a bug where the status never
> changes as expected; but I guess CI bots have timeouts at higher layers that
> would detect if such a bug sneaks in.

Although it might be useful to make sure when such a timeout lands, you
know which state you thought you were waiting for.

Dave

> > +
> >   index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
> >   class QMPTestCase(unittest.TestCase):
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 2/4] iotests.py: Add wait_for_runstate()

[Vladimir Sementsov-Ogievskiy]

20.08.2020 17:23, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> On 8/18/20 8:32 AM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>    tests/qemu-iotests/iotests.py | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 717b5b652c..ee93cf22db 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -833,6 +833,10 @@ class VM(qtest.QEMUQtestMachine):
>>>                       'Found node %s under %s (but expected %s)' % \
>>>                       (node['name'], path, expected_node)
>>> +    def wait_for_runstate(self, runstate: str) -> None:
>>> +        while self.qmp('query-status')['return']['status'] != runstate:
>>> +            time.sleep(0.2)
>>
>> This looks like it could inf-loop if we have a bug where the status never
>> changes as expected; but I guess CI bots have timeouts at higher layers that
>> would detect if such a bug sneaks in.
> 
> Although it might be useful to make sure when such a timeout lands, you
> know which state you thought you were waiting for.
> 
> Dave
> 

Timeout class is defined in iotests.py, so we can simply insert something like

  ... , timeout=60) -> None:
   with Timeout(timeout, f"Timeout waiting for '{runstate}' runstate"):
      ...


-- 
Best regards,
Vladimir

Re: [PATCH v4 2/4] iotests.py: Add wait_for_runstate()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1776 bytes --]

On 20.08.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
> 20.08.2020 17:23, Dr. David Alan Gilbert wrote:
>> * Eric Blake (eblake@redhat.com) wrote:
>>> On 8/18/20 8:32 AM, Max Reitz wrote:
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/iotests.py | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>>>
>>>> diff --git a/tests/qemu-iotests/iotests.py
>>>> b/tests/qemu-iotests/iotests.py
>>>> index 717b5b652c..ee93cf22db 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>> @@ -833,6 +833,10 @@ class VM(qtest.QEMUQtestMachine):
>>>>                       'Found node %s under %s (but expected %s)' % \
>>>>                       (node['name'], path, expected_node)
>>>> +    def wait_for_runstate(self, runstate: str) -> None:
>>>> +        while self.qmp('query-status')['return']['status'] !=
>>>> runstate:
>>>> +            time.sleep(0.2)
>>>
>>> This looks like it could inf-loop if we have a bug where the status
>>> never
>>> changes as expected; but I guess CI bots have timeouts at higher
>>> layers that
>>> would detect if such a bug sneaks in.
>>
>> Although it might be useful to make sure when such a timeout lands, you
>> know which state you thought you were waiting for.
>>
>> Dave
>>
> 
> Timeout class is defined in iotests.py, so we can simply insert
> something like
> 
>  ... , timeout=60) -> None:
>   with Timeout(timeout, f"Timeout waiting for '{runstate}' runstate"):
>      ...

Actually, I’m not even using this function here in v4 anymore, so this
patch might as well be dropped.

Max


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

Re: [PATCH v4 2/4] iotests.py: Add wait_for_runstate()

[Vladimir Sementsov-Ogievskiy]

18.08.2020 16:32, Max Reitz wrote:
> Signed-off-by: Max Reitz<mreitz@redhat.com>

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

-- 
Best regards,
Vladimir

[PATCH v4 3/4] iotests.py: Let wait_migration() return on failure

[Max Reitz]

Let wait_migration() return on failure (with the return value indicating
whether the migration was completed or has failed), so we can use it for
migrations that are expected to fail, too.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ee93cf22db..f39fd580a6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -729,16 +729,22 @@ class VM(qtest.QEMUQtestMachine):
             }
         ]))
 
-    def wait_migration(self, expect_runstate):
+    def wait_migration(self, expect_runstate: Optional[str]) -> bool:
         while True:
             event = self.event_wait('MIGRATION')
             log(event, filters=[filter_qmp_event])
-            if event['data']['status'] == 'completed':
+            if event['data']['status'] in ('completed', 'failed'):
                 break
-        # The event may occur in finish-migrate, so wait for the expected
-        # post-migration runstate
-        while self.qmp('query-status')['return']['status'] != expect_runstate:
-            pass
+
+        if event['data']['status'] == 'completed':
+            # The event may occur in finish-migrate, so wait for the expected
+            # post-migration runstate
+            runstate = None
+            while runstate != expect_runstate:
+                runstate = self.qmp('query-status')['return']['status']
+            return True
+        else:
+            return False
 
     def node_info(self, node_name):
         nodes = self.qmp('query-named-block-nodes')
-- 
2.26.2

Re: [PATCH v4 3/4] iotests.py: Let wait_migration() return on failure

[Eric Blake]

On 8/18/20 8:32 AM, Max Reitz wrote:
> Let wait_migration() return on failure (with the return value indicating
> whether the migration was completed or has failed), so we can use it for
> migrations that are expected to fail, too.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 

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

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

Re: [PATCH v4 3/4] iotests.py: Let wait_migration() return on failure

[Vladimir Sementsov-Ogievskiy]

18.08.2020 16:32, Max Reitz wrote:
> Let wait_migration() return on failure (with the return value indicating
> whether the migration was completed or has failed), so we can use it for
> migrations that are expected to fail, too.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index ee93cf22db..f39fd580a6 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -729,16 +729,22 @@ class VM(qtest.QEMUQtestMachine):
>               }
>           ]))
>   
> -    def wait_migration(self, expect_runstate):
> +    def wait_migration(self, expect_runstate: Optional[str]) -> bool:
>           while True:
>               event = self.event_wait('MIGRATION')
>               log(event, filters=[filter_qmp_event])
> -            if event['data']['status'] == 'completed':
> +            if event['data']['status'] in ('completed', 'failed'):
>                   break
> -        # The event may occur in finish-migrate, so wait for the expected
> -        # post-migration runstate
> -        while self.qmp('query-status')['return']['status'] != expect_runstate:
> -            pass
> +
> +        if event['data']['status'] == 'completed':
> +            # The event may occur in finish-migrate, so wait for the expected
> +            # post-migration runstate
> +            runstate = None
> +            while runstate != expect_runstate:
> +                runstate = self.qmp('query-status')['return']['status']

Would be good to use the helper from the previous patch. With it or not:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +            return True
> +        else:
> +            return False
>   
>       def node_info(self, node_name):
>           nodes = self.qmp('query-named-block-nodes')
> 


-- 
Best regards,
Vladimir

[PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 601 insertions(+)
 create mode 100755 tests/qemu-iotests/300
 create mode 100644 tests/qemu-iotests/300.out

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
new file mode 100755
index 0000000000..c6d86b1dbc
--- /dev/null
+++ b/tests/qemu-iotests/300
@@ -0,0 +1,595 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# Tests for dirty bitmaps migration with node aliases
+#
+# 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 os
+import random
+import re
+import time
+from typing import Dict, List, Optional, Union
+import iotests
+import qemu
+
+BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
+
+assert iotests.sock_dir is not None
+mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
+
+class TestDirtyBitmapMigration(iotests.QMPTestCase):
+    src_node_name: str = ''
+    dst_node_name: str = ''
+    src_bmap_name: str = ''
+    dst_bmap_name: str = ''
+
+    def setUp(self) -> None:
+        self.vm_a = iotests.VM(path_suffix='-a')
+        self.vm_a.add_blockdev(f'node-name={self.src_node_name},' \
+                               'driver=null-co')
+        self.vm_a.launch()
+
+        self.vm_b = iotests.VM(path_suffix='-b')
+        self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' \
+                               'driver=null-co')
+        self.vm_b.add_incoming(f'unix:{mig_sock}')
+        self.vm_b.launch()
+
+        result = self.vm_a.qmp('block-dirty-bitmap-add',
+                               node=self.src_node_name,
+                               name=self.src_bmap_name)
+        self.assert_qmp(result, 'return', {})
+
+        # Dirty some random megabytes
+        for _ in range(9):
+            mb_ofs = random.randrange(1024)
+            self.vm_a.hmp_qemu_io(self.src_node_name, f'write {mb_ofs}M 1M')
+
+        result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
+                               node=self.src_node_name,
+                               name=self.src_bmap_name)
+        self.bitmap_hash_reference = result['return']['sha256']
+
+        caps = [{'capability': name, 'state': True} \
+                for name in ('dirty-bitmaps', 'events')]
+
+        for vm in (self.vm_a, self.vm_b):
+            result = vm.qmp('migrate-set-capabilities', capabilities=caps)
+            self.assert_qmp(result, 'return', {})
+
+    def tearDown(self) -> None:
+        self.vm_a.shutdown()
+        self.vm_b.shutdown()
+        try:
+            os.remove(mig_sock)
+        except OSError:
+            pass
+
+    def check_bitmap(self, bitmap_name_valid: bool) -> None:
+        result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
+                               node=self.dst_node_name,
+                               name=self.dst_bmap_name)
+        if bitmap_name_valid:
+            self.assert_qmp(result, 'return/sha256',
+                            self.bitmap_hash_reference)
+        else:
+            self.assert_qmp(result, 'error/desc',
+                            f"Dirty bitmap '{self.dst_bmap_name}' not found")
+
+    def migrate(self, bitmap_name_valid: bool = True,
+                migration_success: bool = True) -> None:
+        result = self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
+        self.assert_qmp(result, 'return', {})
+
+        with iotests.Timeout(5, 'Timeout waiting for migration to complete'):
+            self.assertEqual(self.vm_a.wait_migration('postmigrate'),
+                             migration_success)
+            self.assertEqual(self.vm_b.wait_migration('running'),
+                             migration_success)
+
+        if migration_success:
+            self.check_bitmap(bitmap_name_valid)
+
+    def verify_dest_error(self, msg: Optional[str]) -> None:
+        """
+        Check whether the given error message is present in vm_b's log.
+        (vm_b is shut down to do so.)
+        If @msg is None, check that there has not been any error.
+        """
+        self.vm_b.shutdown()
+        if msg is None:
+            self.assertNotIn('qemu-system-', self.vm_b.get_log())
+        else:
+            self.assertIn(msg, self.vm_b.get_log())
+
+    @staticmethod
+    def mapping(node_name: str, node_alias: str,
+                bitmap_name: str, bitmap_alias: str) \
+        -> BlockBitmapMapping:
+        return [{
+            'node-name': node_name,
+            'alias': node_alias,
+            'bitmaps': [{
+                'name': bitmap_name,
+                'alias': bitmap_alias
+            }]
+        }]
+
+    def set_mapping(self, vm: iotests.VM, mapping: BlockBitmapMapping,
+                    error: Optional[str] = None) \
+        -> None:
+        """
+        Invoke migrate-set-parameters on @vm to set the given @mapping.
+        Check for success if @error is None, or verify the error message
+        if it is not.
+        On success, verify that "info migrate_parameters" on HMP returns
+        our mapping.  (Just to check its formatting code.)
+        """
+        result = vm.qmp('migrate-set-parameters',
+                        block_bitmap_mapping=mapping)
+
+        if error is None:
+            self.assert_qmp(result, 'return', {})
+
+            result = vm.qmp('human-monitor-command',
+                            command_line='info migrate_parameters')
+
+            hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
+                                    result['return'], flags=re.MULTILINE)
+
+            self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
+                             self.to_hmp_mapping(mapping))
+        else:
+            self.assert_qmp(result, 'error/desc', error)
+
+    @staticmethod
+    def to_hmp_mapping(mapping: BlockBitmapMapping) -> str:
+        result = 'block-bitmap-mapping:\n'
+
+        for node in mapping:
+            result += f"  '{node['node-name']}' -> '{node['alias']}'\n"
+
+            assert isinstance(node['bitmaps'], list)
+            for bitmap in node['bitmaps']:
+                result += f"    '{bitmap['name']}' -> '{bitmap['alias']}'\n"
+
+        return result
+
+
+class TestAliasMigration(TestDirtyBitmapMigration):
+    src_node_name = 'node0'
+    dst_node_name = 'node0'
+    src_bmap_name = 'bmap0'
+    dst_bmap_name = 'bmap0'
+
+    def test_migration_without_alias(self) -> None:
+        self.migrate(self.src_node_name == self.dst_node_name and
+                     self.src_bmap_name == self.dst_bmap_name)
+
+        # Check for error message on the destination
+        if self.src_node_name != self.dst_node_name:
+            self.verify_dest_error(f"Cannot find " \
+                                   f"device={self.src_node_name} nor " \
+                                   f"node_name={self.src_node_name}")
+        else:
+            self.verify_dest_error(None)
+
+    def test_alias_on_src_migration(self) -> None:
+        mapping = self.mapping(self.src_node_name, self.dst_node_name,
+                               self.src_bmap_name, self.dst_bmap_name)
+
+        self.set_mapping(self.vm_a, mapping)
+        self.migrate()
+        self.verify_dest_error(None)
+
+    def test_alias_on_dst_migration(self) -> None:
+        mapping = self.mapping(self.dst_node_name, self.src_node_name,
+                               self.dst_bmap_name, self.src_bmap_name)
+
+        self.set_mapping(self.vm_b, mapping)
+        self.migrate()
+        self.verify_dest_error(None)
+
+    def test_alias_on_both_migration(self) -> None:
+        src_map = self.mapping(self.src_node_name, 'node-alias',
+                               self.src_bmap_name, 'bmap-alias')
+
+        dst_map = self.mapping(self.dst_node_name, 'node-alias',
+                               self.dst_bmap_name, 'bmap-alias')
+
+        self.set_mapping(self.vm_a, src_map)
+        self.set_mapping(self.vm_b, dst_map)
+        self.migrate()
+        self.verify_dest_error(None)
+
+
+class TestNodeAliasMigration(TestAliasMigration):
+    src_node_name = 'node-src'
+    dst_node_name = 'node-dst'
+
+
+class TestBitmapAliasMigration(TestAliasMigration):
+    src_bmap_name = 'bmap-src'
+    dst_bmap_name = 'bmap-dst'
+
+
+class TestFullAliasMigration(TestAliasMigration):
+    src_node_name = 'node-src'
+    dst_node_name = 'node-dst'
+    src_bmap_name = 'bmap-src'
+    dst_bmap_name = 'bmap-dst'
+
+
+class TestLongBitmapNames(TestAliasMigration):
+    # Giving long bitmap names is OK, as long as there is a short alias for
+    # migration
+    src_bmap_name = 'a' * 512
+    dst_bmap_name = 'b' * 512
+
+    # Skip all tests that do not use the intermediate alias
+    def test_migration_without_alias(self) -> None:
+        pass
+
+    def test_alias_on_src_migration(self) -> None:
+        pass
+
+    def test_alias_on_dst_migration(self) -> None:
+        pass
+
+
+class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
+    src_node_name = 'node0'
+    dst_node_name = 'node0'
+    src_bmap_name = 'bmap0'
+    dst_bmap_name = 'bmap0'
+
+    """
+    Note that mapping nodes or bitmaps that do not exist is not an error.
+    """
+
+    def test_non_injective_node_mapping(self) -> None:
+        mapping: BlockBitmapMapping = [
+            {
+                'node-name': 'node0',
+                'alias': 'common-alias',
+                'bitmaps': [{
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                }]
+            },
+            {
+                'node-name': 'node1',
+                'alias': 'common-alias',
+                'bitmaps': [{
+                    'name': 'bmap1',
+                    'alias': 'bmap-alias1'
+                }]
+            }
+        ]
+
+        self.set_mapping(self.vm_a, mapping,
+                         "Invalid mapping given for block-bitmap-mapping: " \
+                         "The node alias 'common-alias' is used twice")
+
+    def test_non_injective_bitmap_mapping(self) -> None:
+        mapping: BlockBitmapMapping = [{
+            'node-name': 'node0',
+            'alias': 'node-alias0',
+            'bitmaps': [
+                {
+                    'name': 'bmap0',
+                    'alias': 'common-alias'
+                },
+                {
+                    'name': 'bmap1',
+                    'alias': 'common-alias'
+                }
+            ]
+        }]
+
+        self.set_mapping(self.vm_a, mapping,
+                         "Invalid mapping given for block-bitmap-mapping: " \
+                         "The bitmap alias 'node-alias0'/'common-alias' is " \
+                         "used twice")
+
+    def test_ambiguous_node_mapping(self) -> None:
+        mapping: BlockBitmapMapping = [
+            {
+                'node-name': 'node0',
+                'alias': 'node-alias0',
+                'bitmaps': [{
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                }]
+            },
+            {
+                'node-name': 'node0',
+                'alias': 'node-alias1',
+                'bitmaps': [{
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                }]
+            }
+        ]
+
+        self.set_mapping(self.vm_a, mapping,
+                         "Invalid mapping given for block-bitmap-mapping: " \
+                         "The node name 'node0' is mapped twice")
+
+    def test_ambiguous_bitmap_mapping(self) -> None:
+        mapping: BlockBitmapMapping = [{
+            'node-name': 'node0',
+            'alias': 'node-alias0',
+            'bitmaps': [
+                {
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                },
+                {
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias1'
+                }
+            ]
+        }]
+
+        self.set_mapping(self.vm_a, mapping,
+                         "Invalid mapping given for block-bitmap-mapping: " \
+                         "The bitmap 'node0'/'bmap0' is mapped twice")
+
+    def test_migratee_node_is_not_mapped_on_src(self) -> None:
+        self.set_mapping(self.vm_a, [])
+        # Should just ignore all bitmaps on unmapped nodes
+        self.migrate(False)
+        self.verify_dest_error(None)
+
+    def test_migratee_node_is_not_mapped_on_dst(self) -> None:
+        self.set_mapping(self.vm_b, [])
+        self.migrate(False)
+        self.verify_dest_error(f"Unknown node alias '{self.src_node_name}'")
+
+    def test_migratee_bitmap_is_not_mapped_on_src(self) -> None:
+        mapping: BlockBitmapMapping = [{
+            'node-name': self.src_node_name,
+            'alias': self.dst_node_name,
+            'bitmaps': []
+        }]
+
+        self.set_mapping(self.vm_a, mapping)
+        # Should just ignore all unmapped bitmaps
+        self.migrate(False)
+        self.verify_dest_error(None)
+
+    def test_migratee_bitmap_is_not_mapped_on_dst(self) -> None:
+        mapping: BlockBitmapMapping = [{
+            'node-name': self.dst_node_name,
+            'alias': self.src_node_name,
+            'bitmaps': []
+        }]
+
+        self.set_mapping(self.vm_b, mapping)
+        self.migrate(False)
+        self.verify_dest_error(f"Unknown bitmap alias " \
+                               f"'{self.src_bmap_name}' " \
+                               f"on node '{self.dst_node_name}' " \
+                               f"(alias '{self.src_node_name}')")
+
+    def test_unused_mapping_on_dst(self) -> None:
+        # Let the source not send any bitmaps
+        self.set_mapping(self.vm_a, [])
+
+        # Establish some mapping on the destination
+        self.set_mapping(self.vm_b, [])
+
+        # The fact that there is a mapping on B without any bitmaps
+        # being received should be fine, not fatal
+        self.migrate(False)
+        self.verify_dest_error(None)
+
+    def test_non_wellformed_node_alias(self) -> None:
+        alias = '123-foo'
+
+        mapping: BlockBitmapMapping = [{
+            'node-name': self.src_node_name,
+            'alias': alias,
+            'bitmaps': []
+        }]
+
+        self.set_mapping(self.vm_a, mapping,
+                         f"Invalid mapping given for block-bitmap-mapping: " \
+                         f"The node alias '{alias}' is not well-formed")
+
+    def test_node_alias_too_long(self) -> None:
+        alias = 'a' * 256
+
+        mapping: BlockBitmapMapping = [{
+            'node-name': self.src_node_name,
+            'alias': alias,
+            'bitmaps': []
+        }]
+
+        self.set_mapping(self.vm_a, mapping,
+                         f"Invalid mapping given for block-bitmap-mapping: " \
+                         f"The node alias '{alias}' is longer than 255 bytes")
+
+    def test_bitmap_alias_too_long(self) -> None:
+        alias = 'a' * 256
+
+        mapping = self.mapping(self.src_node_name, self.dst_node_name,
+                               self.src_bmap_name, alias)
+
+        self.set_mapping(self.vm_a, mapping,
+                         f"Invalid mapping given for block-bitmap-mapping: " \
+                         f"The bitmap alias '{alias}' is longer than 255 " \
+                         f"bytes")
+
+    def test_bitmap_name_too_long(self) -> None:
+        name = 'a' * 256
+
+        result = self.vm_a.qmp('block-dirty-bitmap-add',
+                               node=self.src_node_name,
+                               name=name)
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate(False, False)
+
+        # Check for the error in the source's log
+        self.vm_a.shutdown()
+        self.assertIn(f"Cannot migrate bitmap '{name}' on node " \
+                      f"'{self.src_node_name}': Name is longer than 255 bytes",
+                      self.vm_a.get_log())
+
+        # Expect abnormal shutdown of the destination VM because of
+        # the failed migration
+        try:
+            self.vm_b.shutdown()
+        except qemu.machine.AbnormalShutdown:
+            pass
+
+    def test_aliased_bitmap_name_too_long(self) -> None:
+        # Longer than the maximum for bitmap names
+        self.dst_bmap_name = 'a' * 1024
+
+        mapping = self.mapping(self.dst_node_name, self.src_node_name,
+                               self.dst_bmap_name, self.src_bmap_name)
+
+        # We would have to create this bitmap during migration, and
+        # that would fail, because the name is too long.  Better to
+        # catch it early.
+        self.set_mapping(self.vm_b, mapping,
+                         f"Invalid mapping given for block-bitmap-mapping: " \
+                         f"The bitmap name '{self.dst_bmap_name}' is longer " \
+                         f"than 1023 bytes")
+
+    def test_node_name_too_long(self) -> None:
+        # Longer than the maximum for node names
+        self.dst_node_name = 'a' * 32
+
+        mapping = self.mapping(self.dst_node_name, self.src_node_name,
+                               self.dst_bmap_name, self.src_bmap_name)
+
+        # During migration, this would appear simply as a node that
+        # cannot be found.  Still better to catch impossible node
+        # names early (similar to test_non_wellformed_node_alias).
+        self.set_mapping(self.vm_b, mapping,
+                         f"Invalid mapping given for block-bitmap-mapping: " \
+                         f"The node name '{self.dst_node_name}' is longer " \
+                         f"than 31 bytes")
+
+
+class TestCrossAliasMigration(TestDirtyBitmapMigration):
+    """
+    Swap aliases, both to see that qemu does not get confused, and
+    that we can migrate multiple things at once.
+
+    So we migrate this:
+      node-a.bmap-a -> node-b.bmap-b
+      node-a.bmap-b -> node-b.bmap-a
+      node-b.bmap-a -> node-a.bmap-b
+      node-b.bmap-b -> node-a.bmap-a
+    """
+
+    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')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm_b.qmp('blockdev-add',
+                               node_name='node-a', driver='null-co')
+        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 cross_mapping() -> BlockBitmapMapping:
+        return [
+            {
+                'node-name': 'node-a',
+                'alias': 'node-b',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-b'
+                    },
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-a'
+                    }
+                ]
+            },
+            {
+                'node-name': 'node-b',
+                'alias': 'node-a',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-a'
+                    },
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-b'
+                    }
+                ]
+            }
+        ]
+
+    def verify_dest_has_all_bitmaps(self) -> None:
+        bitmaps = self.vm_b.query_bitmaps()
+
+        # Extract and sort bitmap names
+        for node in bitmaps:
+            bitmaps[node] = sorted((bmap['name'] for bmap in bitmaps[node]))
+
+        self.assertEqual(bitmaps,
+                         {'node-a': ['bmap-a', 'bmap-b'],
+                          'node-b': ['bmap-a', 'bmap-b']})
+
+    def test_alias_on_src(self) -> None:
+        self.set_mapping(self.vm_a, self.cross_mapping())
+
+        # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
+        # that is enough
+        self.migrate()
+        self.verify_dest_has_all_bitmaps()
+        self.verify_dest_error(None)
+
+    def test_alias_on_dst(self) -> None:
+        self.set_mapping(self.vm_b, self.cross_mapping())
+
+        # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
+        # that is enough
+        self.migrate()
+        self.verify_dest_has_all_bitmaps()
+        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
new file mode 100644
index 0000000000..cafb8161f7
--- /dev/null
+++ b/tests/qemu-iotests/300.out
@@ -0,0 +1,5 @@
+.....................................
+----------------------------------------------------------------------
+Ran 37 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1dc034d3af..17fdc2cdaa 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -308,6 +308,7 @@
 296 rw
 297 meta
 299 auto quick
+300 migration
 301 backing quick
 302 quick
 304 rw quick
-- 
2.26.2

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Eric Blake]

On 8/18/20 8:32 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/300.out |   5 +

Rather sparse output (I hate debugging those sorts of outputs when the 
test is failing).

>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 601 insertions(+)
>   create mode 100755 tests/qemu-iotests/300
>   create mode 100644 tests/qemu-iotests/300.out
> 

> +        # Dirty some random megabytes
> +        for _ in range(9):
> +            mb_ofs = random.randrange(1024)
> +            self.vm_a.hmp_qemu_io(self.src_node_name, f'write {mb_ofs}M 1M')

It turns out that the discard operation likewise dirties the bitmap, but 
slightly faster (see edb90bbd).  We could optimize it on top, but I'm 
not going to require a micro-optimizing to get it in.  The test takes 
about 12 seconds to run for me, but you didn't mark it as such in 
'group', so that's good; but it turns up a problem:

300      fail       [20:55:54] [20:56:06]                    output 
mismatch (see 300.out.bad)
--- /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out	2020-08-19 
20:53:11.087791988 -0500
+++ /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out.bad	2020-08-19 
20:56:06.092428756 -0500
@@ -1,5 +1,41 @@
-.....................................
+WARNING:qemu.machine:qemu received signal 11; command: 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 
-display none -vga none -chardev 
socket,id=mon,path=/tmp/tmp.qT831UThme/qemu-b-798452-monitor.sock -mon 
chardev=mon,mode=control -qtest 
unix:path=/tmp/tmp.qT831UThme/qemu-b-798452-qtest.sock -accel qtest 
-nodefaults -display none -accel qtest -blockdev 
node-name=node0,driver=null-co -incoming unix:/tmp/tmp.qT831UThme/mig_sock"
+.............FE.......................
+======================================================================
+ERROR: test_migratee_bitmap_is_not_mapped_on_dst 
(__main__.TestBlockBitmapMappingErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 435, in _do_shutdown
+    self._soft_shutdown(timeout, has_quit)
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 415, in _soft_shutdown
+    self._qmp.cmd('quit')
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py", 
line 266, in cmd
+    return self.cmd_obj(qmp_cmd)
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py", 
line 246, in cmd_obj
+    self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
+BrokenPipeError: [Errno 32] Broken pipe
+
+The above exception was the direct cause of the following exception:
+
+Traceback (most recent call last):
+  File "300", line 76, in tearDown
+    self.vm_b.shutdown()
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 465, in shutdown
+    self._do_shutdown(timeout, has_quit)
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 438, in _do_shutdown
+    raise AbnormalShutdown("Could not perform graceful shutdown") \
+qemu.machine.AbnormalShutdown: Could not perform graceful shutdown
+
+======================================================================
+FAIL: test_migratee_bitmap_is_not_mapped_on_dst 
(__main__.TestBlockBitmapMappingErrors)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "300", line 384, in test_migratee_bitmap_is_not_mapped_on_dst
+    self.migrate(False)
+  File "300", line 99, in migrate
+    self.assertEqual(self.vm_a.wait_migration('postmigrate'),
+AssertionError: False != True
+
  ----------------------------------------------------------------------
  Ran 37 tests

-OK
+FAILED (failures=1, errors=1)

I'm not sure why I'm seeing that, but it looks like you've got a bad 
deref somewhere in the alias code.

> +class TestLongBitmapNames(TestAliasMigration):
> +    # Giving long bitmap names is OK, as long as there is a short alias for
> +    # migration
> +    src_bmap_name = 'a' * 512
> +    dst_bmap_name = 'b' * 512

This part's new compared to v3 ;)  Looks like you've made several 
enhancements.

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

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5671 bytes --]

On 20.08.20 03:58, Eric Blake wrote:
> On 8/18/20 8:32 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/300.out |   5 +
> 
> Rather sparse output (I hate debugging those sorts of outputs when the
> test is failing).

Hm.  I don’t know, the stack trace usually gives a good idea and
./check -d gives QMP context.

The advantage of a sparse output is that we don’t need to adjust the
reference output every time some optional field is added somewhere.

>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 601 insertions(+)
>>   create mode 100755 tests/qemu-iotests/300
>>   create mode 100644 tests/qemu-iotests/300.out
>>
> 
>> +        # Dirty some random megabytes
>> +        for _ in range(9):
>> +            mb_ofs = random.randrange(1024)
>> +            self.vm_a.hmp_qemu_io(self.src_node_name, f'write
>> {mb_ofs}M 1M')
> 
> It turns out that the discard operation likewise dirties the bitmap, but
> slightly faster (see edb90bbd).  We could optimize it on top, but I'm
> not going to require a micro-optimizing to get it in.  The test takes
> about 12 seconds to run for me, but you didn't mark it as such in
> 'group', so that's good; but it turns up a problem:
> 
> 300      fail       [20:55:54] [20:56:06]                    output
> mismatch (see 300.out.bad)
> --- /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out    2020-08-19
> 20:53:11.087791988 -0500
> +++ /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out.bad    2020-08-19
> 20:56:06.092428756 -0500
> @@ -1,5 +1,41 @@
> -.....................................
> +WARNING:qemu.machine:qemu received signal 11; command:
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64
> -display none -vga none -chardev
> socket,id=mon,path=/tmp/tmp.qT831UThme/qemu-b-798452-monitor.sock -mon
> chardev=mon,mode=control -qtest
> unix:path=/tmp/tmp.qT831UThme/qemu-b-798452-qtest.sock -accel qtest
> -nodefaults -display none -accel qtest -blockdev
> node-name=node0,driver=null-co -incoming unix:/tmp/tmp.qT831UThme/mig_sock"
> +.............FE.......................
> +======================================================================
> +ERROR: test_migratee_bitmap_is_not_mapped_on_dst
> (__main__.TestBlockBitmapMappingErrors)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> 435, in _do_shutdown
> +    self._soft_shutdown(timeout, has_quit)
> +  File
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> 415, in _soft_shutdown
> +    self._qmp.cmd('quit')
> +  File
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py",
> line 266, in cmd
> +    return self.cmd_obj(qmp_cmd)
> +  File
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py",
> line 246, in cmd_obj
> +    self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
> +BrokenPipeError: [Errno 32] Broken pipe
> +
> +The above exception was the direct cause of the following exception:
> +
> +Traceback (most recent call last):
> +  File "300", line 76, in tearDown
> +    self.vm_b.shutdown()
> +  File
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> 465, in shutdown
> +    self._do_shutdown(timeout, has_quit)
> +  File
> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> 438, in _do_shutdown
> +    raise AbnormalShutdown("Could not perform graceful shutdown") \
> +qemu.machine.AbnormalShutdown: Could not perform graceful shutdown
> +
> +======================================================================
> +FAIL: test_migratee_bitmap_is_not_mapped_on_dst
> (__main__.TestBlockBitmapMappingErrors)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "300", line 384, in test_migratee_bitmap_is_not_mapped_on_dst
> +    self.migrate(False)
> +  File "300", line 99, in migrate
> +    self.assertEqual(self.vm_a.wait_migration('postmigrate'),
> +AssertionError: False != True
> +
>  ----------------------------------------------------------------------
>  Ran 37 tests
> 
> -OK
> +FAILED (failures=1, errors=1)
> 
> I'm not sure why I'm seeing that, but it looks like you've got a bad
> deref somewhere in the alias code.

Ah, crap.

This should fix it:

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 89cb16b12c..d407dfefea 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1091,7 +1091,9 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
             } else {
                 bitmap_name = s->bitmap_alias;
             }
+        }

+        if (!s->cancelled) {
             g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
             s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);


I had this originally, and then I decided to drop that hunk just before
sending v4 because I couldn’t see the point.  But we need it, because if
the bitmap alias is unknown, the migration is cancelled, so we need to
re-check s->cancalled after the alias lookup block.

Would you be OK with squashing that into patch 1?

Max


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

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Vladimir Sementsov-Ogievskiy]

20.08.2020 16:17, Max Reitz wrote:
> On 20.08.20 03:58, Eric Blake wrote:
>> On 8/18/20 8:32 AM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>    tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
>>>    tests/qemu-iotests/300.out |   5 +
>>
>> Rather sparse output (I hate debugging those sorts of outputs when the
>> test is failing).
> 
> Hm.  I don’t know, the stack trace usually gives a good idea and
> ./check -d gives QMP context.
> 
> The advantage of a sparse output is that we don’t need to adjust the
> reference output every time some optional field is added somewhere.
> 
>>>    tests/qemu-iotests/group   |   1 +
>>>    3 files changed, 601 insertions(+)
>>>    create mode 100755 tests/qemu-iotests/300
>>>    create mode 100644 tests/qemu-iotests/300.out
>>>
>>
>>> +        # Dirty some random megabytes
>>> +        for _ in range(9):
>>> +            mb_ofs = random.randrange(1024)
>>> +            self.vm_a.hmp_qemu_io(self.src_node_name, f'write
>>> {mb_ofs}M 1M')
>>
>> It turns out that the discard operation likewise dirties the bitmap, but
>> slightly faster (see edb90bbd).  We could optimize it on top, but I'm
>> not going to require a micro-optimizing to get it in.  The test takes
>> about 12 seconds to run for me, but you didn't mark it as such in
>> 'group', so that's good; but it turns up a problem:
>>
>> 300      fail       [20:55:54] [20:56:06]                    output
>> mismatch (see 300.out.bad)
>> --- /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out    2020-08-19
>> 20:53:11.087791988 -0500
>> +++ /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out.bad    2020-08-19
>> 20:56:06.092428756 -0500
>> @@ -1,5 +1,41 @@
>> -.....................................
>> +WARNING:qemu.machine:qemu received signal 11; command:
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64
>> -display none -vga none -chardev
>> socket,id=mon,path=/tmp/tmp.qT831UThme/qemu-b-798452-monitor.sock -mon
>> chardev=mon,mode=control -qtest
>> unix:path=/tmp/tmp.qT831UThme/qemu-b-798452-qtest.sock -accel qtest
>> -nodefaults -display none -accel qtest -blockdev
>> node-name=node0,driver=null-co -incoming unix:/tmp/tmp.qT831UThme/mig_sock"
>> +.............FE.......................
>> +======================================================================
>> +ERROR: test_migratee_bitmap_is_not_mapped_on_dst
>> (__main__.TestBlockBitmapMappingErrors)
>> +----------------------------------------------------------------------
>> +Traceback (most recent call last):
>> +  File
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
>> 435, in _do_shutdown
>> +    self._soft_shutdown(timeout, has_quit)
>> +  File
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
>> 415, in _soft_shutdown
>> +    self._qmp.cmd('quit')
>> +  File
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py",
>> line 266, in cmd
>> +    return self.cmd_obj(qmp_cmd)
>> +  File
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py",
>> line 246, in cmd_obj
>> +    self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
>> +BrokenPipeError: [Errno 32] Broken pipe
>> +
>> +The above exception was the direct cause of the following exception:
>> +
>> +Traceback (most recent call last):
>> +  File "300", line 76, in tearDown
>> +    self.vm_b.shutdown()
>> +  File
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
>> 465, in shutdown
>> +    self._do_shutdown(timeout, has_quit)
>> +  File
>> "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
>> 438, in _do_shutdown
>> +    raise AbnormalShutdown("Could not perform graceful shutdown") \
>> +qemu.machine.AbnormalShutdown: Could not perform graceful shutdown
>> +
>> +======================================================================
>> +FAIL: test_migratee_bitmap_is_not_mapped_on_dst
>> (__main__.TestBlockBitmapMappingErrors)
>> +----------------------------------------------------------------------
>> +Traceback (most recent call last):
>> +  File "300", line 384, in test_migratee_bitmap_is_not_mapped_on_dst
>> +    self.migrate(False)
>> +  File "300", line 99, in migrate
>> +    self.assertEqual(self.vm_a.wait_migration('postmigrate'),
>> +AssertionError: False != True
>> +
>>   ----------------------------------------------------------------------
>>   Ran 37 tests
>>
>> -OK
>> +FAILED (failures=1, errors=1)
>>
>> I'm not sure why I'm seeing that, but it looks like you've got a bad
>> deref somewhere in the alias code.
> 
> Ah, crap.
> 
> This should fix it:
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 89cb16b12c..d407dfefea 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -1091,7 +1091,9 @@ static int dirty_bitmap_load_header(QEMUFile *f,
> DBMLoadState *s,
>               } else {
>                   bitmap_name = s->bitmap_alias;
>               }
> +        }
> 
> +        if (!s->cancelled) {
>               g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>               s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
> 
> 

That's correct thing to do

> I had this originally, and then I decided to drop that hunk just before
> sending v4 because I couldn’t see the point.  But we need it, because if
> the bitmap alias is unknown, the migration is cancelled, so we need to
> re-check s->cancalled after the alias lookup block.
> 
> Would you be OK with squashing that into patch 1?
> 

Ok for me.

-- 
Best regards,
Vladimir

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Vladimir Sementsov-Ogievskiy]

# MYPYPATH=../../python/ mypy 300
300:154: error: Item "None" of "Optional[Match[Any]]" has no attribute "group"
Found 1 error in 1 file (checked 1 source file)

- the only complain. Suggest a fix:

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index c6d86b1dbc..0241903743 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -148,11 +148,11 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
              result = vm.qmp('human-monitor-command',
                              command_line='info migrate_parameters')
  
-            hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
-                                    result['return'], flags=re.MULTILINE)
+            m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
+                          result['return'], flags=re.MULTILINE)
+            hmp_mapping = m.group(0).replace('\r', '') if m else None
  
-            self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
-                             self.to_hmp_mapping(mapping))
+            self.assertEqual(hmp_mapping, self.to_hmp_mapping(mapping))
          else:
              self.assert_qmp(result, 'error/desc', error)
  
===

# flake8 300
300:24:1: F401 'time' imported but unused
300:34:1: E302 expected 2 blank lines, found 1
300:42:67: E502 the backslash is redundant between brackets
300:47:67: E502 the backslash is redundant between brackets
300:67:53: E502 the backslash is redundant between brackets
300:122:9: E125 continuation line with same indent as next logical line
300:134:9: E125 continuation line with same indent as next logical line
300:185:52: E502 the backslash is redundant between brackets
300:186:72: E502 the backslash is redundant between brackets
300:285:77: E502 the backslash is redundant between brackets
300:305:77: E502 the backslash is redundant between brackets
300:306:78: E502 the backslash is redundant between brackets
300:330:77: E502 the backslash is redundant between brackets
300:350:77: E502 the backslash is redundant between brackets
300:385:57: E502 the backslash is redundant between brackets
300:386:59: E502 the backslash is redundant between brackets
300:387:67: E502 the backslash is redundant between brackets
300:412:78: E502 the backslash is redundant between brackets
300:425:78: E502 the backslash is redundant between brackets
300:435:78: E502 the backslash is redundant between brackets
300:436:76: E502 the backslash is redundant between brackets
300:451:66: E502 the backslash is redundant between brackets
300:473:78: E502 the backslash is redundant between brackets
300:474:79: E502 the backslash is redundant between brackets
300:488:78: E502 the backslash is redundant between brackets
300:489:77: E502 the backslash is redundant between brackets

- I post it just because ALE plugin in vim highlights all these things for me. Up to you, I don't ask you to fix it.

18.08.2020 16:32, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/300.out |   5 +
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 601 insertions(+)
>   create mode 100755 tests/qemu-iotests/300
>   create mode 100644 tests/qemu-iotests/300.out
> 

[..]

> +    def test_alias_on_both_migration(self) -> None:
> +        src_map = self.mapping(self.src_node_name, 'node-alias',
> +                               self.src_bmap_name, 'bmap-alias')
> +
> +        dst_map = self.mapping(self.dst_node_name, 'node-alias',
> +                               self.dst_bmap_name, 'bmap-alias')
> +
> +        self.set_mapping(self.vm_a, src_map)
> +        self.set_mapping(self.vm_b, dst_map)
> +        self.migrate()
> +        self.verify_dest_error(None)

Hmm, probably verify_dest_error() should be called from migrate(), as you call it (almost) always after migrate()

> +
> +

[..]

> +    def test_unused_mapping_on_dst(self) -> None:
> +        # Let the source not send any bitmaps
> +        self.set_mapping(self.vm_a, [])
> +
> +        # Establish some mapping on the destination
> +        self.set_mapping(self.vm_b, [])

Seems, you wanted to specify non-empty mapping for vm_b, yes?
With any non-empty mapping here, just to better correspond to the comments:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

(or keep this case with both mappings empty, and add one similar with empty mapping only on src)



-- 
Best regards,
Vladimir

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Eric Blake]

On 8/20/20 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> # MYPYPATH=../../python/ mypy 300
> 300:154: error: Item "None" of "Optional[Match[Any]]" has no attribute 
> "group"
> Found 1 error in 1 file (checked 1 source file)
> 
> - the only complain. Suggest a fix:
> 
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> index c6d86b1dbc..0241903743 100755
> --- a/tests/qemu-iotests/300
> +++ b/tests/qemu-iotests/300
> @@ -148,11 +148,11 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>               result = vm.qmp('human-monitor-command',
>                               command_line='info migrate_parameters')
> 
> -            hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n  
> .*)*\n',
> -                                    result['return'], flags=re.MULTILINE)
> +            m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
> +                          result['return'], flags=re.MULTILINE)
> +            hmp_mapping = m.group(0).replace('\r', '') if m else None
> 
> -            self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
> -                             self.to_hmp_mapping(mapping))
> +            self.assertEqual(hmp_mapping, self.to_hmp_mapping(mapping))
>           else:
>               self.assert_qmp(result, 'error/desc', error)

Thanks; I'll fold that in to v5.

> 
> ===
> 
> # flake8 300
> 300:24:1: F401 'time' imported but unused
> 300:34:1: E302 expected 2 blank lines, found 1
> 300:42:67: E502 the backslash is redundant between brackets
> 300:47:67: E502 the backslash is redundant between brackets
> 300:67:53: E502 the backslash is redundant between brackets
> 300:122:9: E125 continuation line with same indent as next logical line
> 300:134:9: E125 continuation line with same indent as next logical line
> 300:185:52: E502 the backslash is redundant between brackets
> 300:186:72: E502 the backslash is redundant between brackets
> 300:285:77: E502 the backslash is redundant between brackets
> 300:305:77: E502 the backslash is redundant between brackets
> 300:306:78: E502 the backslash is redundant between brackets
> 300:330:77: E502 the backslash is redundant between brackets
> 300:350:77: E502 the backslash is redundant between brackets
> 300:385:57: E502 the backslash is redundant between brackets
> 300:386:59: E502 the backslash is redundant between brackets
> 300:387:67: E502 the backslash is redundant between brackets
> 300:412:78: E502 the backslash is redundant between brackets
> 300:425:78: E502 the backslash is redundant between brackets
> 300:435:78: E502 the backslash is redundant between brackets
> 300:436:76: E502 the backslash is redundant between brackets
> 300:451:66: E502 the backslash is redundant between brackets
> 300:473:78: E502 the backslash is redundant between brackets
> 300:474:79: E502 the backslash is redundant between brackets
> 300:488:78: E502 the backslash is redundant between brackets
> 300:489:77: E502 the backslash is redundant between brackets
> 
> - I post it just because ALE plugin in vim highlights all these things 
> for me. Up to you, I don't ask you to fix it.

Seems like an easy enough touchup to make.


>> +    def test_alias_on_both_migration(self) -> None:
>> +        src_map = self.mapping(self.src_node_name, 'node-alias',
>> +                               self.src_bmap_name, 'bmap-alias')
>> +
>> +        dst_map = self.mapping(self.dst_node_name, 'node-alias',
>> +                               self.dst_bmap_name, 'bmap-alias')
>> +
>> +        self.set_mapping(self.vm_a, src_map)
>> +        self.set_mapping(self.vm_b, dst_map)
>> +        self.migrate()
>> +        self.verify_dest_error(None)
> 
> Hmm, probably verify_dest_error() should be called from migrate(), as 
> you call it (almost) always after migrate()

This one I did not touch; it can be a followup patch if desired.

> 
>> +
>> +
> 
> [..]
> 
>> +    def test_unused_mapping_on_dst(self) -> None:
>> +        # Let the source not send any bitmaps
>> +        self.set_mapping(self.vm_a, [])
>> +
>> +        # Establish some mapping on the destination
>> +        self.set_mapping(self.vm_b, [])
> 
> Seems, you wanted to specify non-empty mapping for vm_b, yes?
> With any non-empty mapping here, just to better correspond to the comments:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> (or keep this case with both mappings empty, and add one similar with 
> empty mapping only on src)

Adding another case will now have to be a separate patch.  At any rate, 
I've taken the liberty of including your R-b in my staging tree for the 
pending pull request, let me know if I should drop it.

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

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Vladimir Sementsov-Ogievskiy]

21.08.2020 03:44, Eric Blake wrote:
> On 8/20/20 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> # MYPYPATH=../../python/ mypy 300
>> 300:154: error: Item "None" of "Optional[Match[Any]]" has no attribute "group"
>> Found 1 error in 1 file (checked 1 source file)
>>
>> - the only complain. Suggest a fix:
>>
>> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
>> index c6d86b1dbc..0241903743 100755
>> --- a/tests/qemu-iotests/300
>> +++ b/tests/qemu-iotests/300
>> @@ -148,11 +148,11 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>>               result = vm.qmp('human-monitor-command',
>>                               command_line='info migrate_parameters')
>>
>> -            hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n .*)*\n',
>> -                                    result['return'], flags=re.MULTILINE)
>> +            m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
>> +                          result['return'], flags=re.MULTILINE)
>> +            hmp_mapping = m.group(0).replace('\r', '') if m else None
>>
>> -            self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
>> -                             self.to_hmp_mapping(mapping))
>> +            self.assertEqual(hmp_mapping, self.to_hmp_mapping(mapping))
>>           else:
>>               self.assert_qmp(result, 'error/desc', error)
> 
> Thanks; I'll fold that in to v5.
> 
>>
>> ===
>>
>> # flake8 300
>> 300:24:1: F401 'time' imported but unused
>> 300:34:1: E302 expected 2 blank lines, found 1
>> 300:42:67: E502 the backslash is redundant between brackets
>> 300:47:67: E502 the backslash is redundant between brackets
>> 300:67:53: E502 the backslash is redundant between brackets
>> 300:122:9: E125 continuation line with same indent as next logical line
>> 300:134:9: E125 continuation line with same indent as next logical line
>> 300:185:52: E502 the backslash is redundant between brackets
>> 300:186:72: E502 the backslash is redundant between brackets
>> 300:285:77: E502 the backslash is redundant between brackets
>> 300:305:77: E502 the backslash is redundant between brackets
>> 300:306:78: E502 the backslash is redundant between brackets
>> 300:330:77: E502 the backslash is redundant between brackets
>> 300:350:77: E502 the backslash is redundant between brackets
>> 300:385:57: E502 the backslash is redundant between brackets
>> 300:386:59: E502 the backslash is redundant between brackets
>> 300:387:67: E502 the backslash is redundant between brackets
>> 300:412:78: E502 the backslash is redundant between brackets
>> 300:425:78: E502 the backslash is redundant between brackets
>> 300:435:78: E502 the backslash is redundant between brackets
>> 300:436:76: E502 the backslash is redundant between brackets
>> 300:451:66: E502 the backslash is redundant between brackets
>> 300:473:78: E502 the backslash is redundant between brackets
>> 300:474:79: E502 the backslash is redundant between brackets
>> 300:488:78: E502 the backslash is redundant between brackets
>> 300:489:77: E502 the backslash is redundant between brackets
>>
>> - I post it just because ALE plugin in vim highlights all these things for me. Up to you, I don't ask you to fix it.
> 
> Seems like an easy enough touchup to make.
> 
> 
>>> +    def test_alias_on_both_migration(self) -> None:
>>> +        src_map = self.mapping(self.src_node_name, 'node-alias',
>>> +                               self.src_bmap_name, 'bmap-alias')
>>> +
>>> +        dst_map = self.mapping(self.dst_node_name, 'node-alias',
>>> +                               self.dst_bmap_name, 'bmap-alias')
>>> +
>>> +        self.set_mapping(self.vm_a, src_map)
>>> +        self.set_mapping(self.vm_b, dst_map)
>>> +        self.migrate()
>>> +        self.verify_dest_error(None)
>>
>> Hmm, probably verify_dest_error() should be called from migrate(), as you call it (almost) always after migrate()
> 
> This one I did not touch; it can be a followup patch if desired.
> 
>>
>>> +
>>> +
>>
>> [..]
>>
>>> +    def test_unused_mapping_on_dst(self) -> None:
>>> +        # Let the source not send any bitmaps
>>> +        self.set_mapping(self.vm_a, [])
>>> +
>>> +        # Establish some mapping on the destination
>>> +        self.set_mapping(self.vm_b, [])
>>
>> Seems, you wanted to specify non-empty mapping for vm_b, yes?
>> With any non-empty mapping here, just to better correspond to the comments:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> (or keep this case with both mappings empty, and add one similar with empty mapping only on src)
> 
> Adding another case will now have to be a separate patch.  At any rate, I've taken the liberty of including your R-b in my staging tree for the pending pull request, let me know if I should drop it.
> 

OK, thanks, no objections.

-- 
Best regards,
Vladimir

Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3910 bytes --]

On 20.08.20 17:49, Vladimir Sementsov-Ogievskiy wrote:
> # MYPYPATH=../../python/ mypy 300
> 300:154: error: Item "None" of "Optional[Match[Any]]" has no attribute
> "group"
> Found 1 error in 1 file (checked 1 source file)

:(

> - the only complain. Suggest a fix:
> 
> diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> index c6d86b1dbc..0241903743 100755
> --- a/tests/qemu-iotests/300
> +++ b/tests/qemu-iotests/300
> @@ -148,11 +148,11 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>              result = vm.qmp('human-monitor-command',
>                              command_line='info migrate_parameters')
>  
> -            hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n 
> .*)*\n',
> -                                    result['return'], flags=re.MULTILINE)
> +            m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
> +                          result['return'], flags=re.MULTILINE)
> +            hmp_mapping = m.group(0).replace('\r', '') if m else None
>  
> -            self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
> -                             self.to_hmp_mapping(mapping))
> +            self.assertEqual(hmp_mapping, self.to_hmp_mapping(mapping))

Looks good, thanks.

>          else:
>              self.assert_qmp(result, 'error/desc', error)
>  
> ===
> 
> # flake8 300
[...]
> - I post it just because ALE plugin in vim highlights all these things
> for me. Up to you, I don't ask you to fix it.

Thanks, I’ll run it from now on (unless I forget, like mypy above...).

> 18.08.2020 16:32, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/300.out |   5 +
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 601 insertions(+)
>>   create mode 100755 tests/qemu-iotests/300
>>   create mode 100644 tests/qemu-iotests/300.out
>>
> 
> [..]
> 
>> +    def test_alias_on_both_migration(self) -> None:
>> +        src_map = self.mapping(self.src_node_name, 'node-alias',
>> +                               self.src_bmap_name, 'bmap-alias')
>> +
>> +        dst_map = self.mapping(self.dst_node_name, 'node-alias',
>> +                               self.dst_bmap_name, 'bmap-alias')
>> +
>> +        self.set_mapping(self.vm_a, src_map)
>> +        self.set_mapping(self.vm_b, dst_map)
>> +        self.migrate()
>> +        self.verify_dest_error(None)
> 
> Hmm, probably verify_dest_error() should be called from migrate(), as
> you call it (almost) always after migrate()

I don’t know, it shuts down the destination VM, so it would seem a bit
strange to do that as part of migrate().

>> +    def test_unused_mapping_on_dst(self) -> None:
>> +        # Let the source not send any bitmaps
>> +        self.set_mapping(self.vm_a, [])
>> +
>> +        # Establish some mapping on the destination
>> +        self.set_mapping(self.vm_b, [])
> 
> Seems, you wanted to specify non-empty mapping for vm_b, yes?

Oops.  I don’t know how it could have happened that I forgot that.

> With any non-empty mapping here, just to better correspond to the comments:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> (or keep this case with both mappings empty, and add one similar with
> empty mapping only on src)

I’ll see in what shape this patch lands in master, and then I’ll
probably add that other case, yes.

Max


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