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 <farosas@suse.de>
Reviewed-by: Peter Xu <peterx@redhat.com>
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