Re: [PATCH v11 23/27] migration: Capture error in postcopy_ram_listen_thread()

[Arun Menon <armenon@redhat.com>]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 04:57:38PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@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.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@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