Re: [PATCH 4/7] block/dirty-bitmap: Clean up local variable shadowing

[Kevin Wolf <kwolf@xxxxxxxxxx>]

Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
> ---
>  block/monitor/bitmap-qmp-cmds.c | 2 +-
>  block/qcow2-bitmap.c            | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index 55f778f5af..4d018423d8 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char 
> *node, const char *target,
>  
>      for (lst = bms; lst; lst = lst->next) {
>          switch (lst->value->type) {
> -            const char *name, *node;
> +            const char *name;
>          case QTYPE_QSTRING:
>              name = lst->value->u.local;
>              src = bdrv_find_dirty_bitmap(bs, name);

The names in this function are all over the place... A more ambitious
patch could rename the parameters to dst_node/dst_bitmap and these
variables to src_node/src_bitmap to get some more consistency (both with
each other and with the existing src/dst variables).

Preexisting, so I'm not insisting that you should do this.

> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 037fa2d435..ffd5cd3b23 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1555,7 +1555,6 @@ bool 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>      FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>          const char *name = bdrv_dirty_bitmap_name(bitmap);
>          uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> -        Qcow2Bitmap *bm;
>  
>          if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
>              bdrv_dirty_bitmap_inconsistent(bitmap)) {
> @@ -1625,7 +1624,7 @@ bool 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>  
>      /* allocate clusters and store bitmaps */
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
> +        bitmap = bm->dirty_bitmap;
>  
>          if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
>              continue;

Reviewed-by: Kevin Wolf <kwolf@xxxxxxxxxx>