Re: [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
On 10/24/23 13:51, Thomas Huth wrote: On 24/10/2023 12.37, Markus Armbruster wrote: Markus Armbruster writes: Thomas Huth 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 --- 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 And queued. Thanks! Mind if I drop the Fixes: tag? Nothing broken until we enable -Wshadow=local... Fine for me! This looks like the last remaining warning. Are we going to activate -Wshadow=local next ? Thanks, C.
Re: [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
On 24/10/2023 12.37, Markus Armbruster wrote: Markus Armbruster writes: Thomas Huth 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 --- 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 And queued. Thanks! Mind if I drop the Fixes: tag? Nothing broken until we enable -Wshadow=local... Fine for me! Thomas
Re: [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
Markus Armbruster writes: > Thomas Huth 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 >> --- >> 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 > > And queued. Thanks! Mind if I drop the Fixes: tag? Nothing broken until we enable -Wshadow=local...
Re: [PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
Thomas Huth 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 > --- > 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 And queued. Thanks!
[PATCH v2] block/snapshot: Fix compiler warning with -Wshadow=local
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 --- 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(); -- 2.41.0