Re: [PATCH for-11.0 3/6] migration: Make migration_connect_set_error() own the error

[Peter Xu <peterx@redhat.com>]

On Wed, Nov 26, 2025 at 08:27:48AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Make migration_connect_set_error() take ownership of the error always.
> > Paving way for making migrate_set_error() to take ownership.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/channel.c   | 1 -
> >  migration/migration.c | 7 ++++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 462cc183e1..92435fa7f7 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -95,7 +95,6 @@ void migration_channel_connect(MigrationState *s,
> >          }
> >      }
> >      migration_connect(s, error);
> > -    error_free(error);
> >  }
> >  
> >  
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b316ee01ab..4fe69cc2ef 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1575,7 +1575,7 @@ static void migrate_error_free(MigrationState *s)
> >      }
> >  }
> >  
> > -static void migration_connect_set_error(MigrationState *s, const Error *error)
> > +static void migration_connect_set_error(MigrationState *s, Error *error)
> 
> Recommend to rename for the same reason you rename migrate_set_error()
> in the last patch.
> 
> Bonus: all calls become visible in the patch, easing review.

Makes sense, will do.

> 
> >  {
> >      MigrationStatus current = s->state;
> >      MigrationStatus next;
> > @@ -1602,6 +1602,7 @@ static void migration_connect_set_error(MigrationState *s, const Error *error)
> >  
> >      migrate_set_state(&s->state, current, next);
> >      migrate_set_error(s, error);
> > +    error_free(error);
> >  }
> >  
> >  void migration_cancel(void)
> > @@ -2292,7 +2293,7 @@ void qmp_migrate(const char *uri, bool has_channels,
> >  
> >  out:
> >      if (local_err) {
> > -        migration_connect_set_error(s, local_err);
> > +        migration_connect_set_error(s, error_copy(local_err));
> >          error_propagate(errp, local_err);
> >      }
> >  }
> > @@ -2337,7 +2338,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> >          if (!resume_requested) {
> >              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> >          }
> > -        migration_connect_set_error(s, local_err);
> > +        migration_connect_set_error(s, error_copy(local_err));
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> 
> There's one more call in migration_connect().  Doesn't it need an
> error_copy() now?  Oh, I see: it doesn't, because its only caller
> migration_channel_connect() loses its error_free() in the first hunk.
> 
> So, migration_connect() *also* takes ownership now.  The commit message
> should cover this.

Will add a note.

> 
> Aside: I'd be tempted to lift the if (error_in) code from
> migration_connect() to migration_channel_connect() and drop the
> @error_in parameter.

It was deliberately introduced in:

commit 688a3dcba980bf01344a1ae2bc37fea44c6014ac
Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Date:   Fri Dec 15 17:16:55 2017 +0000

    migration: Route errors down through migration_channel_connect

That change will need some justification and care taken on its own when
changing back to before.  I plan to stick with the current goal of this
series so far (which is to remove autoptr for Error and also make the main
migration error API to follow Error's) to land this first.

Thanks!

-- 
Peter Xu