On Wed, Oct 18, 2023 at 09:12:36AM +0200, Juan Quintela wrote:
> Peter Xu <pet...@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 <pet...@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
