migration/ram.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
It's possible that some errors can be overwritten with success retval later
on, and then ignored. Always capture all errors and report.
Reported by Coverity 1522861, but actually I spot one more in the same
function.
Fixes: CID 1522861
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/ram.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index c844151ee9..d8bdb53a8f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
ret = qemu_ram_resize(block, length, &local_err);
if (local_err) {
error_report_err(local_err);
+ assert(ret < 0);
+ return ret;
}
}
/* For postcopy we need to check hugepage sizes match */
@@ -3898,7 +3900,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
error_report("Mismatched RAM page size %s "
"(local) %zd != %" PRId64, block->idstr,
block->page_size, remote_page_size);
- ret = -EINVAL;
+ return -EINVAL;
}
}
if (migrate_ignore_shared()) {
@@ -3908,7 +3910,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
error_report("Mismatched GPAs for block %s "
"%" PRId64 "!= %" PRId64, block->idstr,
(uint64_t)addr, (uint64_t)block->mr->addr);
- ret = -EINVAL;
+ return -EINVAL;
}
}
ret = rdma_block_notification_handle(f, block->idstr);
--
2.41.0
On Tue, 17 Oct 2023 at 21:40, Peter Xu <peterx@redhat.com> wrote: > > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. The other one is CID 1522862, I think. > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/ram.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c844151ee9..d8bdb53a8f 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > ret = qemu_ram_resize(block, length, &local_err); > if (local_err) { > error_report_err(local_err); > + assert(ret < 0); We usually don't bother asserting for this kind of "function reports errors two ways" code. > + return ret; > } thanks -- PMM
On Thu, Oct 19, 2023 at 01:40:29PM +0100, Peter Maydell wrote: > On Tue, 17 Oct 2023 at 21:40, Peter Xu <peterx@redhat.com> wrote: > > > > It's possible that some errors can be overwritten with success retval later > > on, and then ignored. Always capture all errors and report. > > > > Reported by Coverity 1522861, but actually I spot one more in the same > > function. > > The other one is CID 1522862, I think. Yes.. > > > Fixes: CID 1522861 > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > migration/ram.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > index c844151ee9..d8bdb53a8f 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > > ret = qemu_ram_resize(block, length, &local_err); > > if (local_err) { > > error_report_err(local_err); > > + assert(ret < 0); > > We usually don't bother asserting for this kind of "function > reports errors two ways" code. Juan, please feel free to drop the assert() if it's in the queue. After this one lands, I'll send a patch to remove qemu_ram_resize retval and only rely on Error*. Thanks, -- Peter Xu
Peter Xu <peterx@redhat.com> wrote: > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. > > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> queued.
Peter Xu <peterx@redhat.com> wrote: > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. > > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/ram.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c844151ee9..d8bdb53a8f 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > ret = qemu_ram_resize(block, length, &local_err); > if (local_err) { > error_report_err(local_err); > + assert(ret < 0); > + return ret; I hate that assert. If you really want that: if (ret < 0) { error_report_err(local_err); assert(ret < 0); return ret; } Rest of the patch looks ok. Later, Juan.
On Wed, Oct 18, 2023 at 09:12:36AM +0200, Juan Quintela wrote: > Peter Xu <peterx@redhat.com> wrote: > > It's possible that some errors can be overwritten with success retval later > > on, and then ignored. Always capture all errors and report. > > > > Reported by Coverity 1522861, but actually I spot one more in the same > > function. > > > > Fixes: CID 1522861 > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/ram.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > index c844151ee9..d8bdb53a8f 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > > ret = qemu_ram_resize(block, length, &local_err); > > if (local_err) { > > error_report_err(local_err); > > + assert(ret < 0); > > + return ret; > > I hate that assert. If you really want that: Please have a look at qemu_ram_resize(). It only contains two error paths. > > > if (ret < 0) { > error_report_err(local_err); This will be similar to above, if qemu_ram_resize() return <0 with err==NULL, it'll crash in error_report_err() too.. at error_get_pretty(). > assert(ret < 0); This is not necessary.. if in this "if" section. So we can drop it (instead of assert it). > return ret; > } > > Rest of the patch looks ok. I tend to prefer just merging this.. but if you strongly prefer the other way, I can drop the assert(). But then I'll prefer "return -EINVAL" rather than "return ret", if you're fine with it. Thanks, -- Peter Xu
Peter Xu <peterx@redhat.com> writes: > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. > > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
© 2016 - 2024 Red Hat, Inc.