Re: [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Thomas Huth <thuth@redhat.com> writes:
>
>> No need to declare a new variable in the the inner code block
>> here, we can re-use the "ret" variable that has been declared
>> at the beginning of the function. With this change, the code
>> can now be successfully compiled with -Wshadow=local again.
>>
>> Fixes: a32e781838 ("Mark bdrv_snapshot_fallback() and callers GRAPH_RDLOCK")
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  v2: Assign "ret" only in one spot
>>
>>  block/snapshot.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 6e16eb803a..55974273ae 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -629,7 +629,6 @@ int bdrv_all_goto_snapshot(const char *name,
>>      while (iterbdrvs) {
>>          BlockDriverState *bs = iterbdrvs->data;
>>          AioContext *ctx = bdrv_get_aio_context(bs);
>> -        int ret = 0;
>>          bool all_snapshots_includes_bs;
>>  
>>          aio_context_acquire(ctx);
>> @@ -637,9 +636,8 @@ int bdrv_all_goto_snapshot(const char *name,
>>          all_snapshots_includes_bs = bdrv_all_snapshots_includes_bs(bs);
>>          bdrv_graph_rdunlock_main_loop();
>>  
>> -        if (devices || all_snapshots_includes_bs) {
>> -            ret = bdrv_snapshot_goto(bs, name, errp);
>> -        }
>> +        ret = (devices || all_snapshots_includes_bs) ?
>> +              bdrv_snapshot_goto(bs, name, errp) : 0;
>>          aio_context_release(ctx);
>>          if (ret < 0) {
>>              bdrv_graph_rdlock_main_loop();
>
> Better.  Unconditional assignment to @ret right before it's checked is
> how we should use @ret.
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> And queued.  Thanks!

Mind if I drop the Fixes: tag?  Nothing broken until we enable
-Wshadow=local...