Re: [PATCH 6/7] block: 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.c              |  7 ++++---
>  block/rbd.c          |  2 +-
>  block/stream.c       |  1 -
>  block/vvfat.c        | 34 +++++++++++++++++-----------------
>  hw/block/xen-block.c |  6 +++---
>  5 files changed, 25 insertions(+), 25 deletions(-)

I wonder why you made vdi a separate patch, but not vvfat, even though
that has more changes. (Of course, my selfish motivation for asking this
is that I could have given a R-b for it and wouldn't have to look at it
again in a v2 :-))

> diff --git a/block.c b/block.c
> index a307c151a8..7f0003d8ac 100644
> --- a/block.c
> +++ b/block.c
> @@ -3001,7 +3001,8 @@ static BdrvChild 
> *bdrv_attach_child_common(BlockDriverState *child_bs,
>                                             BdrvChildRole child_role,
>                                             uint64_t perm, uint64_t 
> shared_perm,
>                                             void *opaque,
> -                                           Transaction *tran, Error **errp)
> +                                           Transaction *transaction,
> +                                           Error **errp)
>  {
>      BdrvChild *new_child;
>      AioContext *parent_ctx, *new_child_ctx;
> @@ -3088,7 +3089,7 @@ static BdrvChild 
> *bdrv_attach_child_common(BlockDriverState *child_bs,
>          .old_parent_ctx = parent_ctx,
>          .old_child_ctx = child_ctx,
>      };
> -    tran_add(tran, &bdrv_attach_child_common_drv, s);
> +    tran_add(transaction, &bdrv_attach_child_common_drv, s);
>  
>      if (new_child_ctx != child_ctx) {
>          aio_context_release(new_child_ctx);

I think I would resolve this one the other way around. 'tran' is the
typical name for the parameter and it is the transaction that this
function should add things to.

The other one that shadows it is a local transaction that is completed
within the function. I think it's better if that one has a different
name.

As usual, being more specific than just 'tran' vs. 'transaction' would
be nice. Maybe 'aio_ctx_tran' for the nested one?

The rest looks okay.

Kevin