Re: [PULL 23/45] migration: Capture error in postcopy_ram_listen_thread()

[Arun Menon <armenon@redhat.com>]

Thank you for the review.

On Tue, Oct 21, 2025 at 11:37:21AM -0400, Peter Xu wrote:
> On Tue, Oct 21, 2025 at 03:53:11PM +0100, Peter Maydell wrote:
> > On Fri, 3 Oct 2025 at 16:40, Peter Xu <peterx@redhat.com> wrote:
> > >
> > > From: Arun Menon <armenon@redhat.com>
> > >
> > > This is an incremental step in converting vmstate loading
> > > code to report error via Error objects instead of directly
> > > printing it to console/monitor.
> > > postcopy_ram_listen_thread() calls qemu_loadvm_state_main()
> > > to load the vm, and in case of a failure, it should set the error
> > > in the migration object.
> > >
> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > > Signed-off-by: Arun Menon <armenon@redhat.com>
> > > Tested-by: Fabiano Rosas <farosas@suse.de>
> > > Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > > Link: https://lore.kernel.org/r/20250918-propagate_tpm_error-v14-23-36f11a6fb9d3@redhat.com
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > 
> > Hi; Coverity reports a memory leak (CID 1641390) as a
> > result of this change:
> > 
> > >  migration/savevm.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 34b7a28d38..996673b679 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -2095,6 +2095,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > >      QEMUFile *f = mis->from_src_file;
> > >      int load_res;
> > >      MigrationState *migr = migrate_get_current();
> > > +    Error *local_err = NULL;
> > >
> > >      object_ref(OBJECT(migr));
> > >
> > > @@ -2111,7 +2112,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > >      qemu_file_set_blocking(f, true, &error_fatal);
> > >
> > >      /* TODO: sanity check that only postcopiable data will be loaded here */
> > > -    load_res = qemu_loadvm_state_main(f, mis, &error_fatal);
> > > +    load_res = qemu_loadvm_state_main(f, mis, &local_err);
> > 
> > Here, a failure in this function will allocate an Error
> > object and set local_err to point to it.
> > 
> > >
> > >      /*
> > >       * This is tricky, but, mis->from_src_file can change after it
> > > @@ -2137,7 +2138,10 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > >                           __func__, load_res);
> > >              load_res = 0; /* prevent further exit() */
> > >          } else {
> > > -            error_report("%s: loadvm failed: %d", __func__, load_res);
> > > +            error_prepend(&local_err,
> > > +                          "loadvm failed during postcopy: %d: ", load_res);
> > > +            migrate_set_error(migr, local_err);
> > > +            error_report_err(local_err);
> > >              migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > >                                             MIGRATION_STATUS_FAILED);
> > 
> > In this brach of the if(), we error_report_err(), which will
> > free the error object. But in the other branch of the
> > if(), we never do anything with local_err, and so we never free
> > the error object.
> > 
> > I think the true-branch of the if() needs to either
> > incorporate the error into something, or else error_free() it.
> 
> Thanks Peter, I'll sent patches for both issues raised in the pull.

May I take care of it, along with the fix of passing local Error object
into vmstate_load_state()?

> 
> -- 
> Peter Xu
> 

Regards,
Arun Menon