On Fri, Dec 26, 2025 at 06:19:11PM -0300, Fabiano Rosas wrote: > In the next patches migration_cleanup() will be used in qmp_migrate(), > which currently does not show an error message. Move the error > reporting out of migration_cleanup() to avoid duplicated messages. > > Signed-off-by: Fabiano Rosas <[email protected]>
Reviewed-by: Peter Xu <[email protected]> With one comment on top below: > --- > migration/migration.c | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index a56f8fb05e..83854d084a 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1528,10 +1528,6 @@ static void migration_cleanup(MigrationState *s) > MIGRATION_STATUS_CANCELLED); > } > > - if (s->error) { > - /* It is used on info migrate. We can't free it */ > - error_report_err(error_copy(s->error)); > - } > type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED : > MIG_EVENT_PRECOPY_DONE; > migration_call_notifiers(s, type, NULL); > @@ -1540,7 +1536,12 @@ static void migration_cleanup(MigrationState *s) > > static void migration_cleanup_bh(void *opaque) > { > - migration_cleanup(opaque); > + MigrationState *s = opaque; > + > + migration_cleanup(s); > + if (s->error) { > + error_report_err(error_copy(s->error)); > + } > } > > /* > @@ -4026,18 +4027,12 @@ void migration_connect(MigrationState *s, Error > *error_in) > s->expected_downtime = migrate_downtime_limit(); > if (error_in) { > migration_connect_error_propagate(s, error_in); > - if (resume) { > - /* > - * Don't do cleanup for resume if channel is invalid, but only > dump > - * the error. We wait for another channel connect from the user. > - * The error_report still gives HMP user a hint on what failed. > - * It's normally done in migration_cleanup(), but call it here > - * explicitly. > - */ > - error_report_err(error_copy(s->error)); > - } else { > + if (!resume) { > migration_cleanup(s); > } > + if (s->error) { > + error_report_err(error_copy(s->error)); > + } > return; > } > > @@ -4118,6 +4113,9 @@ fail: > } > error_report_err(local_err); [1] > migration_cleanup(s); > + if (s->error) { > + error_report_err(error_copy(s->error)); > + } This should keep no functional difference, which looks correct from that regard. Said that, I wonder if we can drop [1] above, because when reaching here error_in must not be set, so I don't yet see how s->error can be different from local_err. > } > > static void migration_class_init(ObjectClass *klass, const void *data) > -- > 2.51.0 > -- Peter Xu
