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

[Thomas Huth <thuth@redhat.com>]

On 23/10/2023 17.26, Markus Armbruster wrote:
> 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>
>> ---
>>   block/snapshot.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 6e16eb803a..50adf5381e 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -629,8 +629,8 @@ 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;
> 
> Blank line between declarations and statements, please.
> 
> I'm not sure we actually need the assignment.  Proving we don't looks
> like a poor use of our time, though.

I stared at the code for a while, and I think we don't urgently need it. But 
I'd still recommend to rather keep it to render the code more robust for 
future changes (imagine someone adds some code that changes ret in between, 
but does not return on negative values...)

> I recommend to move the assignment from here...
> 
>> +        ret = 0;
>>   
>>           aio_context_acquire(ctx);
>>           bdrv_graph_rdlock_main_loop();
>             all_snapshots_includes_bs = bdrv_all_snapshots_includes_bs(bs);
>             bdrv_graph_rdunlock_main_loop();
> 
> ... down to here.
> 
>             if (devices || all_snapshots_includes_bs) {
>                 ret = bdrv_snapshot_goto(bs, name, errp);
>             }

IMHO this would look best in my eyes:

         ret = (devices || all_snapshots_includes_bs) ?
               bdrv_snapshot_goto(bs, name, errp) : 0;

I'll send a v2 with this change.

>             aio_context_release(ctx);
>             if (ret < 0) {
> 
> We lose the symmetry with the other three while (iterbdrvs) loops.  Do
> we care?

No, at least I don't ;-)

  Thomas