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 <vsement...@virtuozzo.com>
>> Signed-off-by: Max Reitz <mre...@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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
