| Message ID | fc5f27e1fe16cb75e08a248c2d938de3997b9bfb.1613150869.git.pkrempa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: dirty-bitmap: Allow control of bitmap persistence | expand |
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)); } }
On Fri, Feb 12, 2021 at 12:38:10 -0600, Eric Blake wrote: > 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. Anything that gets the patches in :). But honestly, nowadays 80 colums hard limit seems very prehistoric. I understand that shorter lines are better but if you need to hack around it, it IMO defeats the readability of the code anyways.
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index c61d382be8..0244f9bb1d 100644 --- a/migration/block-dirty-bitmap.c +++ 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); amin = g_new(AliasMapInnerNode, 1); *amin = (AliasMapInnerNode){ @@ -276,7 +278,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; + const char *bmap_map_from; if (strlen(bmba->alias) > UINT8_MAX) { error_setg(errp, @@ -293,7 +295,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, if (name_to_alias) { bmap_map_from = bmba->name; - bmap_map_to = bmba->alias; if (g_hash_table_contains(bitmaps_map, bmba->name)) { error_setg(errp, "The bitmap '%s'/'%s' is mapped twice", @@ -302,7 +303,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, } } else { bmap_map_from = bmba->alias; - bmap_map_to = bmba->name; if (g_hash_table_contains(bitmaps_map, bmba->alias)) { error_setg(errp, "The bitmap alias '%s'/'%s' is used twice", @@ -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), + QAPI_CLONE(BitmapMigrationBitmapAlias, bmba)); } } @@ -538,11 +539,15 @@ 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) { + BitmapMigrationBitmapAlias *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->alias; } else { if (strlen(bitmap_name) > UINT8_MAX) { error_report("Cannot migrate bitmap '%s' on node '%s': " @@ -1074,13 +1079,16 @@ 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, - s->bitmap_alias); - if (!bitmap_name) { + BitmapMigrationBitmapAlias *bmap_inner; + + 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, s->bs->node_name, s->node_alias); cancel_incoming_locked(s); + } else { + bitmap_name = bmap_inner->name; } }
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. v3: - use a copy of BitmapMigrationBitmapAlias QAPI sctruct to do the mapping - dropped R-b's because the above is a significant change v2: - NULL-check in freeing function (Eric) - style problems (Vladimir)