Re: [PATCH v3 1/3] migration: dirty-bitmap: Convert alias map inner members to BitmapMigrationBitmapAlias

[Eric Blake]

On 2/12/21 11:34 AM, Peter Krempa wrote:

Long subject line; if it's okay with you, I'd prefer to use:

migration: dirty-bitmap: Use struct for alias map inner members

> 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 pass a copy of the whole
> BitmapMigrationBitmapAlias QAPI struct into the map.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  migration/block-dirty-bitmap.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> Note that there's a very long line but there doesn't seem to be a
> sensible point where to break it.

In other words, the patchew warning can be ignored if I can't reformat
the line.

> +++ b/migration/block-dirty-bitmap.c
> @@ -75,6 +75,8 @@
>  #include "qemu/id.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-migration.h"
> +#include "qapi/qapi-visit-migration.h"
> +#include "qapi/clone-visitor.h"
>  #include "trace.h"
> 
>  #define CHUNK_SIZE     (1 << 10)
> @@ -263,8 +265,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, g_free);
> +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +                                            (GDestroyNotify) 
> qapi_free_BitmapMigrationBitmapAlias);

A possible fix: declare a temporary variable of type GDestroyNotify, so
that assigning the variable uses a shorter line, then use that variable
here.

> @@ -312,7 +312,8 @@ static GHashTable *construct_alias_map(const 
> BitmapMigrationNodeAliasList *bbm,
>              }
> 
>              g_hash_table_insert(bitmaps_map,
> -                                g_strdup(bmap_map_from), 
> g_strdup(bmap_map_to));
> +                                g_strdup(bmap_map_from),

This line could be wrapped with the previous, now.

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

If you like, I can squash the following in before queuing.


diff --git i/migration/block-dirty-bitmap.c w/migration/block-dirty-bitmap.c
index 0244f9bb1d91..e1840a96d8ee 100644
--- i/migration/block-dirty-bitmap.c
+++ w/migration/block-dirty-bitmap.c
@@ -226,6 +226,7 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
         AliasMapInnerNode *amin;
         GHashTable *bitmaps_map;
         const char *node_map_from, *node_map_to;
+        GDestroyNotify gdn;

         if (!id_wellformed(bmna->alias)) {
             error_setg(errp, "The node alias '%s' is not well-formed",
@@ -265,8 +266,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,
-                                            (GDestroyNotify)
qapi_free_BitmapMigrationBitmapAlias);
+        gdn = (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias;
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
g_free, gdn);

         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -311,8 +312,7 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
                 }
             }

-            g_hash_table_insert(bitmaps_map,
-                                g_strdup(bmap_map_from),
+            g_hash_table_insert(bitmaps_map, g_strdup(bmap_map_from),
                                 QAPI_CLONE(BitmapMigrationBitmapAlias,
bmba));
         }
     }

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org