Message ID | 20180619085631.2859-1-aik@ozlabs.ru (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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));
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));
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.
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...
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 --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));
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(-)