* [PATCH] migration/ram: Fix compilation with -Wshadow=local @ 2023-10-23 14:50 Thomas Huth 2023-10-23 15:27 ` Markus Armbruster 2023-10-23 15:57 ` Peter Xu 0 siblings, 2 replies; 9+ messages in thread From: Thomas Huth @ 2023-10-23 14:50 UTC (permalink / raw) To: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas Cc: Leonardo Bras, Markus Armbruster 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); } } -- 2.41.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local 2023-10-23 14:50 [PATCH] migration/ram: Fix compilation with -Wshadow=local Thomas Huth @ 2023-10-23 15:27 ` Markus Armbruster 2023-10-24 4:55 ` Markus Armbruster 2023-10-23 15:57 ` Peter Xu 1 sibling, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2023-10-23 15:27 UTC (permalink / raw) To: Thomas Huth Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras, Markus Armbruster 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! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local 2023-10-23 15:27 ` Markus Armbruster @ 2023-10-24 4:55 ` Markus Armbruster 0 siblings, 0 replies; 9+ messages in thread From: Markus Armbruster @ 2023-10-24 4:55 UTC (permalink / raw) To: Thomas Huth Cc: qemu-devel, Juan Quintela, Peter Xu, Fabiano Rosas, Leonardo Bras 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local 2023-10-23 14:50 [PATCH] migration/ram: Fix compilation with -Wshadow=local Thomas Huth 2023-10-23 15:27 ` Markus Armbruster @ 2023-10-23 15:57 ` Peter Xu 2023-10-23 17:11 ` Thomas Huth 1 sibling, 1 reply; 9+ messages in thread From: Peter Xu @ 2023-10-23 15:57 UTC (permalink / raw) To: Thomas Huth Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras, Markus Armbruster 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 > -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local 2023-10-23 15:57 ` Peter Xu @ 2023-10-23 17:11 ` Thomas Huth 2023-10-23 17:30 ` ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local) Thomas Huth 0 siblings, 1 reply; 9+ messages in thread From: Thomas Huth @ 2023-10-23 17:11 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, Juan Quintela, Fabiano Rosas, Leonardo Bras, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local) 2023-10-23 17:11 ` Thomas Huth @ 2023-10-23 17:30 ` Thomas Huth 2023-10-23 18:55 ` Peter Xu 0 siblings, 1 reply; 9+ messages in thread From: Thomas Huth @ 2023-10-23 17:30 UTC (permalink / raw) To: Peter Xu, Juan Quintela, Markus Armbruster Cc: qemu-devel, Fabiano Rosas, Leonardo Bras 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local) 2023-10-23 17:30 ` ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local) Thomas Huth @ 2023-10-23 18:55 ` Peter Xu 2023-10-24 9:05 ` ram_save_complete() is fishy Thomas Huth 0 siblings, 1 reply; 9+ messages in thread From: Peter Xu @ 2023-10-23 18:55 UTC (permalink / raw) To: Thomas Huth Cc: Juan Quintela, Markus Armbruster, qemu-devel, Fabiano Rosas, Leonardo Bras 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, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ram_save_complete() is fishy 2023-10-23 18:55 ` Peter Xu @ 2023-10-24 9:05 ` Thomas Huth 2023-10-24 13:12 ` Peter Xu 0 siblings, 1 reply; 9+ messages in thread From: Thomas Huth @ 2023-10-24 9:05 UTC (permalink / raw) To: Peter Xu Cc: Juan Quintela, Markus Armbruster, qemu-devel, Fabiano Rosas, Leonardo Bras 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ram_save_complete() is fishy 2023-10-24 9:05 ` ram_save_complete() is fishy Thomas Huth @ 2023-10-24 13:12 ` Peter Xu 0 siblings, 0 replies; 9+ messages in thread From: Peter Xu @ 2023-10-24 13:12 UTC (permalink / raw) To: Thomas Huth Cc: Juan Quintela, Markus Armbruster, qemu-devel, Fabiano Rosas, Leonardo Bras 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! -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-24 13:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-23 14:50 [PATCH] migration/ram: Fix compilation with -Wshadow=local Thomas Huth 2023-10-23 15:27 ` Markus Armbruster 2023-10-24 4:55 ` Markus Armbruster 2023-10-23 15:57 ` Peter Xu 2023-10-23 17:11 ` Thomas Huth 2023-10-23 17:30 ` ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local) Thomas Huth 2023-10-23 18:55 ` Peter Xu 2023-10-24 9:05 ` ram_save_complete() is fishy Thomas Huth 2023-10-24 13:12 ` Peter Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).