migration_completion_precopy() doesn't propagate errors to migration
core which leads to error information loss. Fix that.
This prepares for a follow-up where migration_switchover_start() can
fail on switchover-ack and still report a useful error.
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
migration/savevm.h | 2 +-
migration/migration.c | 13 ++++++++-----
migration/savevm.c | 29 +++++++++++++++++------------
3 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/migration/savevm.h b/migration/savevm.h
index 96fdf96d4e..b6bb4fa977 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -44,7 +44,7 @@ void qemu_savevm_state_header(QEMUFile *f);
int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
void qemu_savevm_state_cleanup(void);
void qemu_savevm_state_complete_postcopy(QEMUFile *f);
-int qemu_savevm_state_complete_precopy(MigrationState *s);
+int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp);
void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index 074d3f2c69..aad23f3228 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2814,7 +2814,7 @@ static bool migration_switchover_start(MigrationState *s, Error **errp)
return true;
}
-static int migration_completion_precopy(MigrationState *s)
+static int migration_completion_precopy(MigrationState *s, Error **errp)
{
int ret;
@@ -2823,16 +2823,17 @@ static int migration_completion_precopy(MigrationState *s)
if (!migrate_mode_is_cpr()) {
ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to stop the VM");
goto out_unlock;
}
}
- if (!migration_switchover_start(s, NULL)) {
+ if (!migration_switchover_start(s, errp)) {
ret = -EFAULT;
goto out_unlock;
}
- ret = qemu_savevm_state_complete_precopy(s);
+ ret = qemu_savevm_state_complete_precopy(s, errp);
out_unlock:
bql_unlock();
return ret;
@@ -2869,7 +2870,7 @@ static void migration_completion(MigrationState *s)
Error *local_err = NULL;
if (s->state == MIGRATION_STATUS_ACTIVE) {
- ret = migration_completion_precopy(s);
+ ret = migration_completion_precopy(s, &local_err);
} else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
migration_completion_postcopy(s);
} else {
@@ -2900,7 +2901,9 @@ static void migration_completion(MigrationState *s)
return;
fail:
- if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
+ if (local_err) {
+ migrate_error_propagate(s, local_err);
+ } else if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
migrate_error_propagate(s, local_err);
} else if (ret) {
error_setg_errno(&local_err, -ret, "Error in migration completion");
diff --git a/migration/savevm.c b/migration/savevm.c
index 23adaf9dd9..9d1d58c8f4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1771,28 +1771,34 @@ int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
return 0;
}
-int qemu_savevm_state_complete_precopy(MigrationState *s)
+int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp)
{
+ ERRP_GUARD();
QEMUFile *f = s->to_dst_file;
- Error *local_err = NULL;
int ret;
ret = qemu_savevm_state_complete_precopy_iterable(f, false);
if (ret) {
+ qemu_file_get_error_obj(f, errp);
+ error_prepend(errp, "Failed to save iterable device state: ");
return ret;
}
- /* TODO: pass error upper */
- ret = qemu_savevm_state_non_iterable(f, &local_err);
+ ret = qemu_savevm_state_non_iterable(f, errp);
if (ret) {
- migrate_error_propagate(s, error_copy(local_err));
- error_report_err(local_err);
return ret;
}
qemu_savevm_state_end_precopy(s, f);
- return qemu_fflush(f);
+ ret = qemu_fflush(f);
+ if (ret) {
+ qemu_file_get_error_obj(f, errp);
+ error_prepend(errp, "%s: Failed to flush QEMUFile", __func__);
+ return ret;
+ }
+
+ return 0;
}
void qemu_savevm_query_pending(MigPendingData *pending, bool exact)
@@ -1874,13 +1880,12 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
}
ret = qemu_file_get_error(f);
- if (ret == 0) {
- qemu_savevm_state_complete_precopy(ms);
- ret = qemu_file_get_error(f);
- }
- if (ret != 0) {
+ if (ret) {
error_setg_errno(errp, -ret, "Error while writing VM state");
+ goto cleanup;
}
+
+ ret = qemu_savevm_state_complete_precopy(ms, errp);
cleanup:
qemu_savevm_state_cleanup();
--
2.40.1On 9/6/26 09:57, Avihai Horon wrote:
> migration_completion_precopy() doesn't propagate errors to migration
> core which leads to error information loss. Fix that.
>
> This prepares for a follow-up where migration_switchover_start() can
> fail on switchover-ack and still report a useful error.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
> migration/savevm.h | 2 +-
> migration/migration.c | 13 ++++++++-----
> migration/savevm.c | 29 +++++++++++++++++------------
> 3 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 96fdf96d4e..b6bb4fa977 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -44,7 +44,7 @@ void qemu_savevm_state_header(QEMUFile *f);
> int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> void qemu_savevm_state_cleanup(void);
> void qemu_savevm_state_complete_postcopy(QEMUFile *f);
> -int qemu_savevm_state_complete_precopy(MigrationState *s);
> +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp);
> void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
> int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
> bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
> diff --git a/migration/migration.c b/migration/migration.c
> index 074d3f2c69..aad23f3228 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2814,7 +2814,7 @@ static bool migration_switchover_start(MigrationState *s, Error **errp)
> return true;
> }
>
> -static int migration_completion_precopy(MigrationState *s)
> +static int migration_completion_precopy(MigrationState *s, Error **errp)
> {
> int ret;
>
> @@ -2823,16 +2823,17 @@ static int migration_completion_precopy(MigrationState *s)
> if (!migrate_mode_is_cpr()) {
> ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
> if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to stop the VM");
> goto out_unlock;
> }
> }
>
> - if (!migration_switchover_start(s, NULL)) {
> + if (!migration_switchover_start(s, errp)) {
> ret = -EFAULT;
This function should now returns a boolean IMHO.
> goto out_unlock;
> }
>
> - ret = qemu_savevm_state_complete_precopy(s);
> + ret = qemu_savevm_state_complete_precopy(s, errp);
> out_unlock:
> bql_unlock();
> return ret;
> @@ -2869,7 +2870,7 @@ static void migration_completion(MigrationState *s)
> Error *local_err = NULL;
>
> if (s->state == MIGRATION_STATUS_ACTIVE) {
> - ret = migration_completion_precopy(s);
> + ret = migration_completion_precopy(s, &local_err);
> } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> migration_completion_postcopy(s);
> } else {
> @@ -2900,7 +2901,9 @@ static void migration_completion(MigrationState *s)
> return;
>
> fail:
> - if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> + if (local_err) {
> + migrate_error_propagate(s, local_err);
> + } else if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
if (local_err
|| qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> migrate_error_propagate(s, local_err);
> } else if (ret) {
> error_setg_errno(&local_err, -ret, "Error in migration completion");
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 23adaf9dd9..9d1d58c8f4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1771,28 +1771,34 @@ int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
> return 0;
> }
>
> -int qemu_savevm_state_complete_precopy(MigrationState *s)
> +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp)
> {
> + ERRP_GUARD();
> QEMUFile *f = s->to_dst_file;
> - Error *local_err = NULL;
> int ret;
>
> ret = qemu_savevm_state_complete_precopy_iterable(f, false);
> if (ret) {
> + qemu_file_get_error_obj(f, errp);
> + error_prepend(errp, "Failed to save iterable device state: ");
> return ret;
> }
>
> - /* TODO: pass error upper */
> - ret = qemu_savevm_state_non_iterable(f, &local_err);
> + ret = qemu_savevm_state_non_iterable(f, errp);
> if (ret) {
> - migrate_error_propagate(s, error_copy(local_err));
> - error_report_err(local_err);
> return ret;
> }
>
> qemu_savevm_state_end_precopy(s, f);
>
> - return qemu_fflush(f);
> + ret = qemu_fflush(f);
> + if (ret) {
> + qemu_file_get_error_obj(f, errp);
> + error_prepend(errp, "%s: Failed to flush QEMUFile", __func__);
Ditto, directly return boolean.
Anyway can be done on top, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> + return ret;
> + }
> +
> + return 0;
> }
>
> void qemu_savevm_query_pending(MigPendingData *pending, bool exact)
> @@ -1874,13 +1880,12 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
> }
>
> ret = qemu_file_get_error(f);
> - if (ret == 0) {
> - qemu_savevm_state_complete_precopy(ms);
> - ret = qemu_file_get_error(f);
> - }
> - if (ret != 0) {
> + if (ret) {
> error_setg_errno(errp, -ret, "Error while writing VM state");
> + goto cleanup;
> }
> +
> + ret = qemu_savevm_state_complete_precopy(ms, errp);
> cleanup:
> qemu_savevm_state_cleanup();
>
On Tue, Jun 09, 2026 at 02:11:00PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/6/26 09:57, Avihai Horon wrote:
> > migration_completion_precopy() doesn't propagate errors to migration
> > core which leads to error information loss. Fix that.
> >
> > This prepares for a follow-up where migration_switchover_start() can
> > fail on switchover-ack and still report a useful error.
> >
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > ---
> > migration/savevm.h | 2 +-
> > migration/migration.c | 13 ++++++++-----
> > migration/savevm.c | 29 +++++++++++++++++------------
> > 3 files changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index 96fdf96d4e..b6bb4fa977 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -44,7 +44,7 @@ void qemu_savevm_state_header(QEMUFile *f);
> > int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> > void qemu_savevm_state_cleanup(void);
> > void qemu_savevm_state_complete_postcopy(QEMUFile *f);
> > -int qemu_savevm_state_complete_precopy(MigrationState *s);
> > +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp);
> > void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
> > int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
> > bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 074d3f2c69..aad23f3228 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2814,7 +2814,7 @@ static bool migration_switchover_start(MigrationState *s, Error **errp)
> > return true;
> > }
> > -static int migration_completion_precopy(MigrationState *s)
> > +static int migration_completion_precopy(MigrationState *s, Error **errp)
> > {
> > int ret;
> > @@ -2823,16 +2823,17 @@ static int migration_completion_precopy(MigrationState *s)
> > if (!migrate_mode_is_cpr()) {
> > ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
> > if (ret < 0) {
> > + error_setg_errno(errp, -ret, "Failed to stop the VM");
> > goto out_unlock;
> > }
> > }
> > - if (!migration_switchover_start(s, NULL)) {
> > + if (!migration_switchover_start(s, errp)) {
> > ret = -EFAULT;
>
> This function should now returns a boolean IMHO.
>
> > goto out_unlock;
> > }
> > - ret = qemu_savevm_state_complete_precopy(s);
> > + ret = qemu_savevm_state_complete_precopy(s, errp);
> > out_unlock:
> > bql_unlock();
> > return ret;
> > @@ -2869,7 +2870,7 @@ static void migration_completion(MigrationState *s)
> > Error *local_err = NULL;
> > if (s->state == MIGRATION_STATUS_ACTIVE) {
> > - ret = migration_completion_precopy(s);
> > + ret = migration_completion_precopy(s, &local_err);
> > } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
> > migration_completion_postcopy(s);
> > } else {
> > @@ -2900,7 +2901,9 @@ static void migration_completion(MigrationState *s)
> > return;
> > fail:
> > - if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> > + if (local_err) {
> > + migrate_error_propagate(s, local_err);
> > + } else if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
>
> if (local_err
> || qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
>
> > migrate_error_propagate(s, local_err);
> > } else if (ret) {
> > error_setg_errno(&local_err, -ret, "Error in migration completion");
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 23adaf9dd9..9d1d58c8f4 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1771,28 +1771,34 @@ int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
> > return 0;
> > }
> > -int qemu_savevm_state_complete_precopy(MigrationState *s)
> > +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp)
> > {
> > + ERRP_GUARD();
> > QEMUFile *f = s->to_dst_file;
> > - Error *local_err = NULL;
> > int ret;
> > ret = qemu_savevm_state_complete_precopy_iterable(f, false);
> > if (ret) {
> > + qemu_file_get_error_obj(f, errp);
> > + error_prepend(errp, "Failed to save iterable device state: ");
> > return ret;
> > }
> > - /* TODO: pass error upper */
> > - ret = qemu_savevm_state_non_iterable(f, &local_err);
> > + ret = qemu_savevm_state_non_iterable(f, errp);
> > if (ret) {
> > - migrate_error_propagate(s, error_copy(local_err));
> > - error_report_err(local_err);
> > return ret;
> > }
> > qemu_savevm_state_end_precopy(s, f);
> > - return qemu_fflush(f);
> > + ret = qemu_fflush(f);
> > + if (ret) {
> > + qemu_file_get_error_obj(f, errp);
> > + error_prepend(errp, "%s: Failed to flush QEMUFile", __func__);
>
> Ditto, directly return boolean.
>
> Anyway can be done on top, so:
Agreed, I didn't mention this because I know touching the retval needs
further touch callers. Can be done on top.
> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
On 6/12/2026 7:03 PM, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, Jun 09, 2026 at 02:11:00PM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/6/26 09:57, Avihai Horon wrote:
>>> migration_completion_precopy() doesn't propagate errors to migration
>>> core which leads to error information loss. Fix that.
>>>
>>> This prepares for a follow-up where migration_switchover_start() can
>>> fail on switchover-ack and still report a useful error.
>>>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> ---
>>> migration/savevm.h | 2 +-
>>> migration/migration.c | 13 ++++++++-----
>>> migration/savevm.c | 29 +++++++++++++++++------------
>>> 3 files changed, 26 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/migration/savevm.h b/migration/savevm.h
>>> index 96fdf96d4e..b6bb4fa977 100644
>>> --- a/migration/savevm.h
>>> +++ b/migration/savevm.h
>>> @@ -44,7 +44,7 @@ void qemu_savevm_state_header(QEMUFile *f);
>>> int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
>>> void qemu_savevm_state_cleanup(void);
>>> void qemu_savevm_state_complete_postcopy(QEMUFile *f);
>>> -int qemu_savevm_state_complete_precopy(MigrationState *s);
>>> +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp);
>>> void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
>>> int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy);
>>> bool qemu_savevm_state_postcopy_prepare(QEMUFile *f, Error **errp);
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 074d3f2c69..aad23f3228 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2814,7 +2814,7 @@ static bool migration_switchover_start(MigrationState *s, Error **errp)
>>> return true;
>>> }
>>> -static int migration_completion_precopy(MigrationState *s)
>>> +static int migration_completion_precopy(MigrationState *s, Error **errp)
>>> {
>>> int ret;
>>> @@ -2823,16 +2823,17 @@ static int migration_completion_precopy(MigrationState *s)
>>> if (!migrate_mode_is_cpr()) {
>>> ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
>>> if (ret < 0) {
>>> + error_setg_errno(errp, -ret, "Failed to stop the VM");
>>> goto out_unlock;
>>> }
>>> }
>>> - if (!migration_switchover_start(s, NULL)) {
>>> + if (!migration_switchover_start(s, errp)) {
>>> ret = -EFAULT;
>> This function should now returns a boolean IMHO.
>>
>>> goto out_unlock;
>>> }
>>> - ret = qemu_savevm_state_complete_precopy(s);
>>> + ret = qemu_savevm_state_complete_precopy(s, errp);
>>> out_unlock:
>>> bql_unlock();
>>> return ret;
>>> @@ -2869,7 +2870,7 @@ static void migration_completion(MigrationState *s)
>>> Error *local_err = NULL;
>>> if (s->state == MIGRATION_STATUS_ACTIVE) {
>>> - ret = migration_completion_precopy(s);
>>> + ret = migration_completion_precopy(s, &local_err);
>>> } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
>>> migration_completion_postcopy(s);
>>> } else {
>>> @@ -2900,7 +2901,9 @@ static void migration_completion(MigrationState *s)
>>> return;
>>> fail:
>>> - if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
>>> + if (local_err) {
>>> + migrate_error_propagate(s, local_err);
>>> + } else if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
>> if (local_err
>> || qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
>>
>>> migrate_error_propagate(s, local_err);
>>> } else if (ret) {
>>> error_setg_errno(&local_err, -ret, "Error in migration completion");
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 23adaf9dd9..9d1d58c8f4 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -1771,28 +1771,34 @@ int qemu_savevm_state_non_iterable(QEMUFile *f, Error **errp)
>>> return 0;
>>> }
>>> -int qemu_savevm_state_complete_precopy(MigrationState *s)
>>> +int qemu_savevm_state_complete_precopy(MigrationState *s, Error **errp)
>>> {
>>> + ERRP_GUARD();
>>> QEMUFile *f = s->to_dst_file;
>>> - Error *local_err = NULL;
>>> int ret;
>>> ret = qemu_savevm_state_complete_precopy_iterable(f, false);
>>> if (ret) {
>>> + qemu_file_get_error_obj(f, errp);
>>> + error_prepend(errp, "Failed to save iterable device state: ");
>>> return ret;
>>> }
>>> - /* TODO: pass error upper */
>>> - ret = qemu_savevm_state_non_iterable(f, &local_err);
>>> + ret = qemu_savevm_state_non_iterable(f, errp);
>>> if (ret) {
>>> - migrate_error_propagate(s, error_copy(local_err));
>>> - error_report_err(local_err);
>>> return ret;
>>> }
>>> qemu_savevm_state_end_precopy(s, f);
>>> - return qemu_fflush(f);
>>> + ret = qemu_fflush(f);
>>> + if (ret) {
>>> + qemu_file_get_error_obj(f, errp);
>>> + error_prepend(errp, "%s: Failed to flush QEMUFile", __func__);
>> Ditto, directly return boolean.
>>
>> Anyway can be done on top, so:
> Agreed, I didn't mention this because I know touching the retval needs
> further touch callers. Can be done on top.
Sure, I can send a patch for that later.
Thanks.
>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> --
> Peter Xu
>
© 2016 - 2026 Red Hat, Inc.