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

[Markus Armbruster <armbru@redhat.com>]

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?