Re: [PATCH v3 01/14] migration: Propagate errors in migration_completion_precopy()

[Peter Xu <peterx@redhat.com>]

On Tue, Jun 09, 2026 at 02:11:00PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/6/26 09:57, Avihai Horon wrote:
> > migration_completion_precopy() doesn't propagate errors to migration
> > core which leads to error information loss. Fix that.
> > 
> > This prepares for a follow-up where migration_switchover_start() can
> > fail on switchover-ack and still report a useful error.
> > 
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > ---
> >   migration/savevm.h    |  2 +-
> >   migration/migration.c | 13 ++++++++-----
> >   migration/savevm.c    | 29 +++++++++++++++++------------
> >   3 files changed, 26 insertions(+), 18 deletions(-)
> > 
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index 96fdf96d4e..b6bb4fa977 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -44,7 +44,7 @@ void qemu_savevm_state_header(QEMUFile *f);
> >   int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> >   void qemu_savevm_state_cleanup(void);
> >   void qemu_savevm_state_complete_postcopy(QEMUFile *f);
> > -int qemu_savevm_state_complete_precopy(MigrationState *s);
> > +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp);
> >   void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
> >   int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
> >   bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 074d3f2c69..aad23f3228 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2814,7 +2814,7 @@ static bool migration_switchover_start(MigrationState *s, Error **errp)
> >       return true;
> >   }
> > -static int migration_completion_precopy(MigrationState *s)
> > +static int migration_completion_precopy(MigrationState *s, Error **errp)
> >   {
> >       int ret;
> > @@ -2823,16 +2823,17 @@ static int migration_completion_precopy(MigrationState *s)
> >       if (!migrate_mode_is_cpr()) {
> >           ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
> >           if (ret < 0) {
> > +            error_setg_errno(errp, -ret, "Failed to stop the VM");
> >               goto out_unlock;
> >           }
> >       }
> > -    if (!migration_switchover_start(s, NULL)) {
> > +    if (!migration_switchover_start(s, errp)) {
> >           ret = -EFAULT;
> 
> This function should now returns a boolean IMHO.
> 
> >           goto out_unlock;
> >       }
> > -    ret = qemu_savevm_state_complete_precopy(s);
> > +    ret = qemu_savevm_state_complete_precopy(s, errp);
> >   out_unlock:
> >       bql_unlock();
> >       return ret;
> > @@ -2869,7 +2870,7 @@ static void migration_completion(MigrationState *s)
> >       Error *local_err = NULL;
> >       if (s->state == MIGRATION_STATUS_ACTIVE) {
> > -        ret = migration_completion_precopy(s);
> > +        ret = migration_completion_precopy(s, &local_err);
> >       } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> >           migration_completion_postcopy(s);
> >       } else {
> > @@ -2900,7 +2901,9 @@ static void migration_completion(MigrationState *s)
> >       return;
> >   fail:
> > -    if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> > +    if (local_err) {
> > +        migrate_error_propagate(s, local_err);
> > +    } else if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> 
>   if (local_err
>       || qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> 
> >           migrate_error_propagate(s, local_err);
> >       } else if (ret) {
> >           error_setg_errno(&local_err, -ret, "Error in migration completion");
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 23adaf9dd9..9d1d58c8f4 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1771,28 +1771,34 @@ int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
> >       return 0;
> >   }
> > -int qemu_savevm_state_complete_precopy(MigrationState *s)
> > +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       QEMUFile *f = s->to_dst_file;
> > -    Error *local_err = NULL;
> >       int ret;
> >       ret = qemu_savevm_state_complete_precopy_iterable(f, false);
> >       if (ret) {
> > +        qemu_file_get_error_obj(f, errp);
> > +        error_prepend(errp, "Failed to save iterable device state: ");
> >           return ret;
> >       }
> > -    /* TODO: pass error upper */
> > -    ret = qemu_savevm_state_non_iterable(f, &local_err);
> > +    ret = qemu_savevm_state_non_iterable(f, errp);
> >       if (ret) {
> > -        migrate_error_propagate(s, error_copy(local_err));
> > -        error_report_err(local_err);
> >           return ret;
> >       }
> >       qemu_savevm_state_end_precopy(s, f);
> > -    return qemu_fflush(f);
> > +    ret = qemu_fflush(f);
> > +    if (ret) {
> > +        qemu_file_get_error_obj(f, errp);
> > +        error_prepend(errp, "%s: Failed to flush QEMUFile", __func__);
> 
> Ditto, directly return boolean.
> 
> Anyway can be done on top, so:

Agreed, I didn't mention this because I know touching the retval needs
further touch callers.  Can be done on top.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu