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

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

[Alexey Kardashevskiy]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Greg Kurz]

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

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

[Markus Armbruster]

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.

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

[Greg Kurz]

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

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