Re: [PATCH 6/7] block: Clean up local variable shadowing

[Markus Armbruster <armbru@xxxxxxxxxx>]

Kevin Wolf <kwolf@xxxxxxxxxx> writes:

> 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 :-))

I split by maintainer.  The files changed by this patch are only covered
by "Block layer core".

>> 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?

Can do.

> The rest looks okay.

Thanks!