| Message ID | 20231023145044.104866-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/ram: Fix compilation with -Wshadow=local | expand |
Thomas Huth <thuth@redhat.com> writes: > No need for a new variable here, especially not for one that shadows > a variable from the beginning of the function scope. With this change > the code now successfully compiles with -Wshadow=local. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > migration/ram.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 92769902bb..9de9e54fa9 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > > ram_flush_compressed_data(rs); > > - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); > - if (ret < 0) { > + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { > qemu_file_set_error(f, ret); > } > } Reviewed-by: Markus Armbruster <armbru@redhat.com> And queued. Thanks!
On Mon, Oct 23, 2023 at 04:50:44PM +0200, Thomas Huth wrote: > No need for a new variable here, especially not for one that shadows > a variable from the beginning of the function scope. With this change > the code now successfully compiles with -Wshadow=local. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > migration/ram.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index 92769902bb..9de9e54fa9 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) > > ram_flush_compressed_data(rs); > > - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); > - if (ret < 0) { > + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { We may need to rename "ret" to something else? qemu_file_set_error(), right below, will reference the error returned. > qemu_file_set_error(f, ret); <----------------- Thanks, > } > } > -- > 2.41.0 >
On 23/10/2023 17.57, Peter Xu wrote: > On Mon, Oct 23, 2023 at 04:50:44PM +0200, Thomas Huth wrote: >> No need for a new variable here, especially not for one that shadows >> a variable from the beginning of the function scope. With this change >> the code now successfully compiles with -Wshadow=local. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> migration/ram.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 92769902bb..9de9e54fa9 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) >> >> ram_flush_compressed_data(rs); >> >> - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); >> - if (ret < 0) { >> + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { > > We may need to rename "ret" to something else? qemu_file_set_error(), > right below, will reference the error returned. > >> qemu_file_set_error(f, ret); <----------------- Oh, drat, right ... that's exactly one of the reasons why shadowing variables is a bad idea ;-) I'll redo a v2. Thomas
On 23/10/2023 19.11, Thomas Huth wrote: > On 23/10/2023 17.57, Peter Xu wrote: >> On Mon, Oct 23, 2023 at 04:50:44PM +0200, Thomas Huth wrote: >>> No need for a new variable here, especially not for one that shadows >>> a variable from the beginning of the function scope. With this change >>> the code now successfully compiles with -Wshadow=local. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> migration/ram.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/migration/ram.c b/migration/ram.c >>> index 92769902bb..9de9e54fa9 100644 >>> --- a/migration/ram.c >>> +++ b/migration/ram.c >>> @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, void >>> *opaque) >>> ram_flush_compressed_data(rs); >>> - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); >>> - if (ret < 0) { >>> + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { >> >> We may need to rename "ret" to something else? qemu_file_set_error(), >> right below, will reference the error returned. >> >>> qemu_file_set_error(f, ret); <----------------- > > Oh, drat, right ... that's exactly one of the reasons why shadowing > variables is a bad idea ;-) > > I'll redo a v2. Actually, there is more fishy stuff in this function: static int ram_save_complete(QEMUFile *f, void *opaque) { ... int ret = 0; ... WITH_RCU_READ_LOCK_GUARD() { ... ret = rdma_registration_start(f, RAM_CONTROL_FINISH); if (ret < 0) { qemu_file_set_error(f, ret); ### here we use the outer "ret" variable ### } ... while (true) { int pages; pages = ram_find_and_save_block(rs); /* no more blocks to sent */ if (pages == 0) { ### here we break without touching "ret" (preserving the previous error) ### break; } if (pages < 0) { ret = pages; ### we only replace the outer "ret" in this break-case here break; } } ... int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); ### so while ret from rdma_registration_start() might be propageted ### below, the ret from rdma_registration_stop() is only local here? if (ret < 0) { qemu_file_set_error(f, ret); } } if (ret < 0) { ### this might trigger by the "ret" from rdma_registration_start() but ### not by the one from rdma_registration_stop()? ... very weird... return ret; } Looks like commit 48408174a7ec7 messed up with the return types pretty badly ... any suggestions what's the right way forward here? Should the return value of rdma_registration_start() only be used for the qemu_file_set_error(), too? Or should the return value of rdma_registration_stop() be allowed to be used for the "return ret" at the end, too? Thomas
On Mon, Oct 23, 2023 at 07:30:04PM +0200, Thomas Huth wrote: > On 23/10/2023 19.11, Thomas Huth wrote: > > On 23/10/2023 17.57, Peter Xu wrote: > > > On Mon, Oct 23, 2023 at 04:50:44PM +0200, Thomas Huth wrote: > > > > No need for a new variable here, especially not for one that shadows > > > > a variable from the beginning of the function scope. With this change > > > > the code now successfully compiles with -Wshadow=local. > > > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > --- > > > > migration/ram.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/migration/ram.c b/migration/ram.c > > > > index 92769902bb..9de9e54fa9 100644 > > > > --- a/migration/ram.c > > > > +++ b/migration/ram.c > > > > @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, > > > > void *opaque) > > > > ram_flush_compressed_data(rs); > > > > - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); > > > > - if (ret < 0) { > > > > + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { > > > > > > We may need to rename "ret" to something else? qemu_file_set_error(), > > > right below, will reference the error returned. > > > > > > > qemu_file_set_error(f, ret); <----------------- > > > > Oh, drat, right ... that's exactly one of the reasons why shadowing > > variables is a bad idea ;-) > > > > I'll redo a v2. > > Actually, there is more fishy stuff in this function: > > static int ram_save_complete(QEMUFile *f, void *opaque) > { > ... > int ret = 0; > ... > WITH_RCU_READ_LOCK_GUARD() { > ... > ret = rdma_registration_start(f, RAM_CONTROL_FINISH); > if (ret < 0) { > qemu_file_set_error(f, ret); > ### here we use the outer "ret" variable ### [1] > } > ... > while (true) { > int pages; > > pages = ram_find_and_save_block(rs); > /* no more blocks to sent */ > if (pages == 0) { > ### here we break without touching "ret" (preserving the previous error) ### > break; > } > if (pages < 0) { > ret = pages; > ### we only replace the outer "ret" in this break-case here > break; > } > } > ... > int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); > ### so while ret from rdma_registration_start() might be propageted > ### below, the ret from rdma_registration_stop() is only local here? > if (ret < 0) { > qemu_file_set_error(f, ret); [2] > } > } > > if (ret < 0) { > ### this might trigger by the "ret" from rdma_registration_start() but > ### not by the one from rdma_registration_stop()? ... very weird... > return ret; > } > > Looks like commit 48408174a7ec7 messed up with the return types pretty badly > ... any suggestions what's the right way forward here? Should the return > value of rdma_registration_start() only be used for the > qemu_file_set_error(), too? Or should the return value of > rdma_registration_stop() be allowed to be used for the "return ret" at the > end, too? Right that's indeed confusing, but it seems confusing too even before that commit. AFAICT, we should "break" for both [1][2] above for any error occured.. Thanks,
Markus Armbruster <armbru@redhat.com> writes: > Thomas Huth <thuth@redhat.com> writes: > >> No need for a new variable here, especially not for one that shadows >> a variable from the beginning of the function scope. With this change >> the code now successfully compiles with -Wshadow=local. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> migration/ram.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index 92769902bb..9de9e54fa9 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) >> >> ram_flush_compressed_data(rs); >> >> - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); >> - if (ret < 0) { >> + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { >> qemu_file_set_error(f, ret); >> } >> } > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > And queued. Thanks! Unqueued.
On 23/10/2023 20.55, Peter Xu wrote: > On Mon, Oct 23, 2023 at 07:30:04PM +0200, Thomas Huth wrote: >> On 23/10/2023 19.11, Thomas Huth wrote: >>> On 23/10/2023 17.57, Peter Xu wrote: >>>> On Mon, Oct 23, 2023 at 04:50:44PM +0200, Thomas Huth wrote: >>>>> No need for a new variable here, especially not for one that shadows >>>>> a variable from the beginning of the function scope. With this change >>>>> the code now successfully compiles with -Wshadow=local. >>>>> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> migration/ram.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/migration/ram.c b/migration/ram.c >>>>> index 92769902bb..9de9e54fa9 100644 >>>>> --- a/migration/ram.c >>>>> +++ b/migration/ram.c >>>>> @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, >>>>> void *opaque) >>>>> ram_flush_compressed_data(rs); >>>>> - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); >>>>> - if (ret < 0) { >>>>> + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { >>>> >>>> We may need to rename "ret" to something else? qemu_file_set_error(), >>>> right below, will reference the error returned. >>>> >>>>> qemu_file_set_error(f, ret); <----------------- >>> >>> Oh, drat, right ... that's exactly one of the reasons why shadowing >>> variables is a bad idea ;-) >>> >>> I'll redo a v2. >> >> Actually, there is more fishy stuff in this function: >> >> static int ram_save_complete(QEMUFile *f, void *opaque) >> { >> ... >> int ret = 0; >> ... >> WITH_RCU_READ_LOCK_GUARD() { >> ... >> ret = rdma_registration_start(f, RAM_CONTROL_FINISH); >> if (ret < 0) { >> qemu_file_set_error(f, ret); >> ### here we use the outer "ret" variable ### > > [1] > >> } >> ... >> while (true) { >> int pages; >> >> pages = ram_find_and_save_block(rs); >> /* no more blocks to sent */ >> if (pages == 0) { >> ### here we break without touching "ret" (preserving the previous error) ### >> break; >> } >> if (pages < 0) { >> ret = pages; >> ### we only replace the outer "ret" in this break-case here >> break; >> } >> } >> ... >> int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); >> ### so while ret from rdma_registration_start() might be propageted >> ### below, the ret from rdma_registration_stop() is only local here? >> if (ret < 0) { >> qemu_file_set_error(f, ret); > > [2] > >> } >> } >> >> if (ret < 0) { >> ### this might trigger by the "ret" from rdma_registration_start() but >> ### not by the one from rdma_registration_stop()? ... very weird... >> return ret; >> } >> >> Looks like commit 48408174a7ec7 messed up with the return types pretty badly >> ... any suggestions what's the right way forward here? Should the return >> value of rdma_registration_start() only be used for the >> qemu_file_set_error(), too? Or should the return value of >> rdma_registration_stop() be allowed to be used for the "return ret" at the >> end, too? > > Right that's indeed confusing, but it seems confusing too even before that > commit. AFAICT, we should "break" for both [1][2] above for any error > occured.. Oh well, looking at the whole file, it seems like most spots that call a rdma_* function just do qemu_file_set_error() afterwards, but then continue with the normal workflow... that looks really confusing to me - if this needs fixing, it should be done by somebody who knows that code better than me, so I'll keep my hands of this and let somebody else fix it if necessary. I'll just respin my original patch to fix the -Wshadow issue. Thomas
On Tue, Oct 24, 2023 at 11:05:12AM +0200, Thomas Huth wrote: > Oh well, looking at the whole file, it seems like most spots that call a > rdma_* function just do qemu_file_set_error() afterwards, but then continue > with the normal workflow... that looks really confusing to me - if this > needs fixing, it should be done by somebody who knows that code better than > me, so I'll keep my hands of this and let somebody else fix it if necessary. > I'll just respin my original patch to fix the -Wshadow issue. Oh.. let me take a closer look today. Thanks for helping, Thomas!
diff --git a/migration/ram.c b/migration/ram.c index 92769902bb..9de9e54fa9 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3238,8 +3238,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque) ram_flush_compressed_data(rs); - int ret = rdma_registration_stop(f, RAM_CONTROL_FINISH); - if (ret < 0) { + if (rdma_registration_stop(f, RAM_CONTROL_FINISH) < 0) { qemu_file_set_error(f, ret); } }
No need for a new variable here, especially not for one that shadows a variable from the beginning of the function scope. With this change the code now successfully compiles with -Wshadow=local. Signed-off-by: Thomas Huth <thuth@redhat.com> --- migration/ram.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)