| Message ID | 20231023175038.111607-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] block/snapshot: Fix compiler warning with -Wshadow=local | expand |
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!
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...
On 24/10/2023 12.37, Markus Armbruster wrote: > 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... Fine for me! Thomas
On 10/24/23 13:51, Thomas Huth wrote: > On 24/10/2023 12.37, Markus Armbruster wrote: >> 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... > > Fine for me! This looks like the last remaining warning. Are we going to activate -Wshadow=local next ? Thanks, C.
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();
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(-)