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