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. -- Peter Xu
