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
-- PMM
