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

[Peter Krempa]

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.