diff mbox

[qemu] xics-kvm: Fix compile warning

Message ID 20180619085631.2859-1-aik@ozlabs.ru (mailing list archive)
State New, archived
Headers show

Commit Message

Alexey Kardashevskiy June 19, 2018, 8:56 a.m. UTC 
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(-)

Comments

David Gibson June 19, 2018, 11:17 a.m. UTC | #1 
On Tue, Jun 19, 2018 at 06:56:31PM +1000, Alexey Kardashevskiy wrote:
> 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>

This no longer applies on ppc-for-3.0.

> ---
>  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));
Greg Kurz June 19, 2018, 11:33 a.m. UTC | #2 
On Tue, 19 Jun 2018 18:56:31 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> 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>
> ---

Already fixed and present in David's last pull req:

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

>  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));
Markus Armbruster June 19, 2018, 11:44 a.m. UTC | #3 
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.

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.
Greg Kurz June 19, 2018, 12:41 p.m. UTC | #4 
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...
Cédric Le Goater June 19, 2018, 12:47 p.m. UTC | #5 
On 06/19/2018 02:41 PM, Greg Kurz wrote:
> 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...
 
It's available on master now. 

C.
diff mbox

Patch

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));