Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu] xics-kvm: Fix compile warning

[Greg Kurz <groug@kaod.org>]

On Tue, 19 Jun 2018 13:44:56 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
> > This fixes uninitialized variable warning:
> >
> > /home/aik/p/qemu/hw/intc/xics_kvm.c: In function ‘ics_set_kvm_state’:
> > /home/aik/p/qemu/hw/intc/xics_kvm.c:281:20: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >              return ret;
> >                     ^~~
> >
> > Discovered with gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0 from Ubuntu 18.04.
> >
> > Fixes: bf358b541b8 "xics_kvm: use KVM helpers"
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> >  hw/intc/xics_kvm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index 8bdf6af..48efbce 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -273,8 +273,8 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
> >                  state |= KVM_XICS_QUEUED;
> >          }
> >  
> > -        kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> > -                          i + ics->offset, &state, true, &local_err);
> > +        ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> > +                                i + ics->offset, &state, true, &local_err);
> >          if (local_err) {
> >              error_report("Unable to restore KVM interrupt controller state"
> >                      " for IRQs %d: %s", i + ics->offset, strerror(errno));  
>                return ret;
>            }
> 
> Unless all callers effectively ignore the return value, this fixes a
> bug, not just a compiler warning.  Recommend to check callers to find
> the bug's impact, and document it in your commit message.
> 

This function has two users:
1) ics_kvm_reset() which ignores it's return value (ie, not impacted)
2) ics_simple_dispatch_post_load() which propagates the return value to
  vmstate_load_state()

If ret is < 0, migration will fail as expected, possibly with an
'Unknow error' message.

If ret >= 0, it will creep up to:

static int
qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
{
[...]
    ret = vmstate_load(f, se);
    if (ret < 0) {
        error_report("error while loading state for instance 0x%x of"
                     " device '%s'", instance_id, idstr);
        return ret;
    }
    if (!check_section_footer(f, se)) {
        return -EINVAL;
    }

    return 0;
}

and migration would likely succeed but leave the guest in an undefined
state.

> Messed up in commit bf358b541b8.  Would be nice to mention that in your
> commit message.
> 
> Also messed up there: leaks local_err.  Please fix that, too.
> 

Both the missing 'ret =' and local_err leak are addressed by:

https://lists.nongnu.org/archive/html/qemu-ppc/2018-06/msg00682.html

It doesn't mention the offending commit though...