On 09/05/23 3:46 pm, Juan Quintela wrote:
> "tejus.gk" <tejus...@nutanix.com> wrote:
>> There are places in the code where the migration is marked failed with
>> MIGRATION_STATUS_FAILED, but the failiure reason is never updated. Hence
>> libvirt doesn't know why the migration failed when it queries for it.
>>
>> Signed-off-by: tejus.gk <tejus...@nutanix.com>
>> ---
>> migration/migration.c | 24 +++++++++++-------------
>> 1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 232e387109..87101eed5c 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1660,15 +1660,9 @@ 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);
>> - }
>
> Why are you removing this yank_unregister()?
As recommended by Daniel in the previous patchset, most of the stuff in this
else block is duplicating the contents of the "if (local_error)" block below
if (local_err) {
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
So now, after local_error gets set through error_setg(), it falls to this block
where yank_unregister() will be called as before.
>
>> - 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) {
>> @@ -2050,7 +2044,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;
>> @@ -2165,7 +2159,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;
>> }
>>
>> @@ -2202,7 +2196,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);
>> }
>> @@ -2719,6 +2713,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);
>> @@ -2741,8 +2736,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;
>> }
>> @@ -3232,8 +3228,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;
>> }
>
> The rest of the patch looks right to me.
>
> Later, Juan.
>
