Re: [PATCH] migration: Fix parse_ramblock() on overwritten retvals

[Peter Xu]

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