On 06/19/2018 02:41 PM, Greg Kurz wrote:
> 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...
It's available on master now.
C.
