On Tue, 19 Jun 2018 13:44:56 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Alexey Kardashevskiy <a...@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 <a...@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...
