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