| Message ID | 20230518062308.90631-2-tejus.gk@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v3,1/2] migration: Update error description whenever migration fails | expand |
Tejus GK <tejus.gk@nutanix.com> wrote: > There are places in migration.c where the migration is marked failed with > MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence > libvirt doesn't know why the migration failed when it queries for it. > > Signed-off-by: Tejus GK <tejus.gk@nutanix.com> Reviewed-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 00d8ba8da0..864150d01d 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1683,15 +1683,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > } else if (strstart(uri, "fd:", &p)) { > fd_start_outgoing_migration(s, p, &local_err); > } else { > - if (!(has_resume && resume)) { > - yank_unregister_instance(MIGRATION_YANK_INSTANCE); > - } > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", > + error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", > "a valid migration protocol"); > migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, > MIGRATION_STATUS_FAILED); > block_cleanup_parameters(); > - return; If you have to respin, please split the removal of the yan_unregister() and return in one patch, and let of this patch the error bits. > } > > if (local_err) { > @@ -2073,7 +2069,7 @@ migration_wait_main_channel(MigrationState *ms) > * Switch from normal iteration to postcopy > * Returns non-0 on error > */ > -static int postcopy_start(MigrationState *ms) > +static int postcopy_start(MigrationState *ms, Error **errp) > { > int ret; > QIOChannelBuffer *bioc; > @@ -2183,7 +2179,7 @@ static int postcopy_start(MigrationState *ms) > */ > ret = qemu_file_get_error(ms->to_dst_file); > if (ret) { > - error_report("postcopy_start: Migration stream errored (pre package)"); > + error_setg(errp, "postcopy_start: Migration stream errored (pre package)"); > goto fail_closefb; > } > > @@ -2220,7 +2216,7 @@ static int postcopy_start(MigrationState *ms) > > ret = qemu_file_get_error(ms->to_dst_file); > if (ret) { > - error_report("postcopy_start: Migration stream errored"); > + error_setg(errp, "postcopy_start: Migration stream errored"); > migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE, > MIGRATION_STATUS_FAILED); > } > @@ -2741,6 +2737,7 @@ typedef enum { > static MigIterateState migration_iteration_run(MigrationState *s) > { > uint64_t must_precopy, can_postcopy; > + Error *local_err = NULL; > bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; > > qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); > @@ -2763,8 +2760,9 @@ static MigIterateState migration_iteration_run(MigrationState *s) > /* Still a significant amount to transfer */ > if (!in_postcopy && must_precopy <= s->threshold_size && > qatomic_read(&s->start_postcopy)) { > - if (postcopy_start(s)) { > - error_report("%s: postcopy failed to start", __func__); > + if (postcopy_start(s, &local_err)) { > + migrate_set_error(s, local_err); > + error_report_err(local_err); Not your fault. But the interface of migrate_set_error() is a mess. It is followed sometimes from error_report_err() another from error_free() and another by ...
On Thu, May 18, 2023 at 06:23:07AM +0000, Tejus GK wrote: > There are places in migration.c where the migration is marked failed with > MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence > libvirt doesn't know why the migration failed when it queries for it. > > Signed-off-by: Tejus GK <tejus.gk@nutanix.com> > --- > migration/migration.c | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
diff --git a/migration/migration.c b/migration/migration.c index 00d8ba8da0..864150d01d 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1683,15 +1683,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } else if (strstart(uri, "fd:", &p)) { fd_start_outgoing_migration(s, p, &local_err); } else { - if (!(has_resume && resume)) { - yank_unregister_instance(MIGRATION_YANK_INSTANCE); - } - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri", + error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol"); migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED); block_cleanup_parameters(); - return; } if (local_err) { @@ -2073,7 +2069,7 @@ migration_wait_main_channel(MigrationState *ms) * Switch from normal iteration to postcopy * Returns non-0 on error */ -static int postcopy_start(MigrationState *ms) +static int postcopy_start(MigrationState *ms, Error **errp) { int ret; QIOChannelBuffer *bioc; @@ -2183,7 +2179,7 @@ static int postcopy_start(MigrationState *ms) */ ret = qemu_file_get_error(ms->to_dst_file); if (ret) { - error_report("postcopy_start: Migration stream errored (pre package)"); + error_setg(errp, "postcopy_start: Migration stream errored (pre package)"); goto fail_closefb; } @@ -2220,7 +2216,7 @@ static int postcopy_start(MigrationState *ms) ret = qemu_file_get_error(ms->to_dst_file); if (ret) { - error_report("postcopy_start: Migration stream errored"); + error_setg(errp, "postcopy_start: Migration stream errored"); migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE, MIGRATION_STATUS_FAILED); } @@ -2741,6 +2737,7 @@ typedef enum { static MigIterateState migration_iteration_run(MigrationState *s) { uint64_t must_precopy, can_postcopy; + Error *local_err = NULL; bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); @@ -2763,8 +2760,9 @@ static MigIterateState migration_iteration_run(MigrationState *s) /* Still a significant amount to transfer */ if (!in_postcopy && must_precopy <= s->threshold_size && qatomic_read(&s->start_postcopy)) { - if (postcopy_start(s)) { - error_report("%s: postcopy failed to start", __func__); + if (postcopy_start(s, &local_err)) { + migrate_set_error(s, local_err); + error_report_err(local_err); } return MIG_ITERATE_SKIP; } @@ -3250,8 +3248,10 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) */ if (migrate_postcopy_ram() || migrate_return_path()) { if (open_return_path_on_source(s, !resume)) { - error_report("Unable to open return-path for postcopy"); + error_setg(&local_err, "Unable to open return-path for postcopy"); migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); + migrate_set_error(s, local_err); + error_report_err(local_err); migrate_fd_cleanup(s); return; } @@ -3275,6 +3275,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) } if (multifd_save_setup(&local_err) != 0) { + migrate_set_error(s, local_err); error_report_err(local_err); migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
There are places in migration.c where the migration is marked failed with MIGRATION_STATUS_FAILED, but the failure reason is never updated. Hence libvirt doesn't know why the migration failed when it queries for it. Signed-off-by: Tejus GK <tejus.gk@nutanix.com> --- migration/migration.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)