Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 04:57:38PM -0300, Fabiano Rosas wrote:
> Arun Menon <arme...@redhat.com> writes:
>
> > 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.lur...@redhat.com>
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/savevm.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > de2bce276faa863a0f25deedafb0b784f10559d7..b85620a03654c214f4e771fa3b2bcfdf48661214
> > 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);
> >
> > /* 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);
> >
> > /*
> > * This is tricky, but, mis->from_src_file can change after it
> > @@ -2137,9 +2138,12 @@ 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);
> > + MIGRATION_STATUS_FAILED);
>
> This should be left alone. Having this patch (git) conflict with
> something else just because of this line would be really annoying.
Sure, will do.
>
> > }
> > }
> > if (load_res >= 0) {
>
Regards,
Arun Menon
