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

[Max Reitz <mreitz@redhat.com>]

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