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 <[email protected]> wrote: > > > > > > From: Arun Menon <[email protected]> > > > > > > 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 <[email protected]> > > > Reviewed-by: Fabiano Rosas <[email protected]> > > > Signed-off-by: Arun Menon <[email protected]> > > > Tested-by: Fabiano Rosas <[email protected]> > > > Reviewed-by: Akihiko Odaki <[email protected]> > > > Link: > > > https://lore.kernel.org/r/[email protected] > > > Signed-off-by: Peter Xu <[email protected]> > > > --- > > > > 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
