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

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(-)

On 2/3/21 6:59 AM, 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);

Do we want to allow free_alias_map_inner_bitmap(NULL)?

Looks like this patch works without it, but it's less future proof, so I
can add that if you agree.

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

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,