[PATCH] migration/ram: Fix compilation with -Wshadow=local

[PATCH] migration/ram: Fix compilation with -Wshadow=local

[Thomas Huth]

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

Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local

[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!

Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local

[Peter Xu]

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

Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local

[Thomas Huth]

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

ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local)

[Thomas Huth]

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

Re: ram_save_complete() is fishy (was: Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local)

[Peter Xu]

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

Re: [PATCH] migration/ram: Fix compilation with -Wshadow=local

[Markus Armbruster]

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.

Re: ram_save_complete() is fishy

[Thomas Huth]

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

Re: ram_save_complete() is fishy

[Peter Xu]

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