On Thu, Jun 04, 2026 at 06:12:11PM +0300, Avihai Horon wrote:
>
> On 6/3/2026 10:47 PM, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Tue, Jun 02, 2026 at 12:26:09PM +0300, 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. Errors from
> > > qemu_savevm_state_complete_precopy() are not propagated yet as it
> > > requires more plumbing.
> > >
> > > Signed-off-by: Avihai Horon <[email protected]>
> > > ---
> > > migration/migration.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 074d3f2c69..7488a94206 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,11 +2823,12 @@ 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;
> > > goto out_unlock;
> > > }
> > IIUC this patch overlooked the follow up call:
> >
> > ret = qemu_savevm_state_complete_precopy(s);
> >
> > We should make sure ret!=0 will always set Error*. Better pass Error**
> > into qemu_savevm_state_complete_precopy() too.
>
> Yes, that was intentional, because this requires more plumbing in
> qemu_savevm_state_complete_precopy() (as mentioned in the commit message).
> Although not clean, I thought it was OK to do this one-time exception since
> we explicitly check for local_err in migration_completion().
>
> I can give it a shot and try adding Error **errp to
> qemu_savevm_state_complete_precopy(), but I think it'll be opening a can of
> worms.
> Do you have any thoughts?
If we want to keep the min change, we can consider setting errp at least
when qemu_savevm_state_complete_precopy() returned non-zero in the path of
migration_completion_precopy(), to make sure errp is always set when error
happened. Otherwise it's error prone.
Thanks,
>
> Thanks.
>
> >
> > Thanks,
> >
> > > @@ -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)) {
> > > migrate_error_propagate(s, local_err);
> > > } else if (ret) {
> > > error_setg_errno(&local_err, -ret, "Error in migration
> > > completion");
> > > --
> > > 2.40.1
> > >
> > --
> > Peter Xu
> >
>
--
Peter Xu
