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

[Cédric Le Goater]

On 06/19/2018 02:41 PM, Greg Kurz wrote:
> On Tue, 19 Jun 2018 13:44:56 +0200
> Markus Armbruster <address@hidden> wrote:
> 
>> Alexey Kardashevskiy <address@hidden> 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 <address@hidden>
>>> ---
>>>  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.