diff mbox series

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

Message ID 8e40a7337e3b9a0a4f11ee3b0e2f3ae4c76f2dbd.1612356810.git.pkrempa@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: dirty-bitmap: Allow control of bitmap persistence on destination | expand

Commit Message

Peter Krempa Feb. 3, 2021, 12:59 p.m. UTC 
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(-)

Comments

Eric Blake Feb. 4, 2021, 7:12 p.m. UTC | #1 
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>
Vladimir Sementsov-Ogievskiy Feb. 5, 2021, 7:04 a.m. UTC | #2 
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,
diff mbox series

Patch

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

         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);
+
+            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);
-            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) {