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

[Eric Blake <eblake@redhat.com>]

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