Re: ram_save_complete() is fishy

[Thomas Huth <thuth@redhat.com>]

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