Re: [PATCH 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct

[Vladimir Sementsov-Ogievskiy]

03.02.2021 15:59, Peter Krempa wrote:
> Currently the alias mapping hash stores just strings of the target
> objects internally. In further patches we'll be adding another member
> which will need to be stored in the map so convert the members to a
> struct.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   migration/block-dirty-bitmap.c | 37 ++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index c61d382be8..b0403dd00c 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -169,6 +169,18 @@ typedef struct DBMState {
>
>   static DBMState dbm_state;
>
> +typedef struct AliasMapInnerBitmap {
> +    char *string;
> +} AliasMapInnerBitmap;
> +
> +static void free_alias_map_inner_bitmap(void *amin_ptr)
> +{
> +    AliasMapInnerBitmap *amin = amin_ptr;
> +
> +    g_free(amin->string);
> +    g_free(amin);
> +}
> +
>   /* For hash tables that map node/bitmap names to aliases */
>   typedef struct AliasMapInnerNode {
>       char *string;
> @@ -264,7 +276,7 @@ static GHashTable *construct_alias_map(const 
> BitmapMigrationNodeAliasList *bbm,
>           }
>
>           bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> -                                            g_free, g_free);
> +                                            g_free, 
> free_alias_map_inner_bitmap);

over-80 line

>           amin = g_new(AliasMapInnerNode, 1);
>           *amin = (AliasMapInnerNode){
> @@ -277,6 +289,7 @@ static GHashTable *construct_alias_map(const 
> BitmapMigrationNodeAliasList *bbm,
>           for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
>               const BitmapMigrationBitmapAlias *bmba = bmbal->value;
>               const char *bmap_map_from, *bmap_map_to;
> +            AliasMapInnerBitmap *bmap_inner;
>
>               if (strlen(bmba->alias) > UINT8_MAX) {
>                   error_setg(errp,
> @@ -311,8 +324,11 @@ static GHashTable *construct_alias_map(const 
> BitmapMigrationNodeAliasList *bbm,
>                   }
>               }
>
> +            bmap_inner = g_new0(AliasMapInnerBitmap, 1);
> +            bmap_inner->string = g_strdup(bmap_map_to);
> +
>               g_hash_table_insert(bitmaps_map,
> -                                g_strdup(bmap_map_from), 
> g_strdup(bmap_map_to));
> +                                g_strdup(bmap_map_from), bmap_inner);
>           }
>       }
>
> @@ -538,11 +554,16 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
> BlockDriverState *bs,
>           }
>
>           if (bitmap_aliases) {
> -            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
> -            if (!bitmap_alias) {
> +            AliasMapInnerBitmap *bmap_inner;
> +
> +            bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name);
> +

I'd drop this line, you are not consistent on it with next hunk anyway)

> +            if (!bmap_inner) {
>                   /* Skip bitmaps with no alias */
>                   continue;
>               }
> +
> +            bitmap_alias = bmap_inner->string;
>           } else {
>               if (strlen(bitmap_name) > UINT8_MAX) {
>                   error_report("Cannot migrate bitmap '%s' on node '%s': "
> @@ -1074,14 +1095,18 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
> DBMLoadState *s,
>
>           bitmap_name = s->bitmap_alias;
>           if (!s->cancelled && bitmap_alias_map) {
> -            bitmap_name = g_hash_table_lookup(bitmap_alias_map,
> +            AliasMapInnerBitmap *bmap_inner;
> +
> +            bmap_inner = g_hash_table_lookup(bitmap_alias_map,
>                                                 s->bitmap_alias);

indentation

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

Looks OK for me (I'm OK with Eric's suggestion too):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

suggest style cleanup:

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b0403dd00c..577e32bf75 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -275,8 +275,8 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,
             node_map_to = bmna->node_name;
         }
 
-        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
-                                            g_free, 
free_alias_map_inner_bitmap);
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+                                            free_alias_map_inner_bitmap);
 
         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -557,7 +557,6 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
BlockDriverState *bs,
             AliasMapInnerBitmap *bmap_inner;
 
             bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name);
-
             if (!bmap_inner) {
                 /* Skip bitmaps with no alias */
                 continue;
@@ -1097,8 +1096,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, 
DBMLoadState *s,
         if (!s->cancelled && bitmap_alias_map) {
             AliasMapInnerBitmap *bmap_inner;
 
-            bmap_inner = g_hash_table_lookup(bitmap_alias_map,
-                                              s->bitmap_alias);
+            bmap_inner = g_hash_table_lookup(bitmap_alias_map, 
s->bitmap_alias);
             if (!bmap_inner) {
                 error_report("Error: Unknown bitmap alias '%s' on node "
                              "'%s' (alias '%s')", s->bitmap_alias,



--
Best regards,
Vladimir