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

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

[Thomas Huth]

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;
+        ret = 0;
 
         aio_context_acquire(ctx);
         bdrv_graph_rdlock_main_loop();
-- 
2.41.0

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

[Markus Armbruster]

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 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);
           }
           aio_context_release(ctx);
           if (ret < 0) {

We lose the symmetry with the other three while (iterbdrvs) loops.  Do
we care?

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

[Thomas Huth]

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