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

[Thomas Huth <thuth@redhat.com>]

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