| Message ID | 20240424174245.1237942-2-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | migration: do not exit on incoming failure | expand |
On Wed, Apr 24, 2024 at 08:42:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 1. Most of callers want to free the error after call. Let's help them. > > 2. Some callers log the error, some not. We also have places where we > log the stored error. Let's instead simply log every migration > error. > > 3. Some callers have to retrieve current migration state only to pass > it to migrate_set_error(). In the new helper let's get the state > automatically. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > migration/migration.c | 48 ++++++++++++---------------------------- > migration/migration.h | 2 +- > migration/multifd.c | 18 ++++++--------- > migration/postcopy-ram.c | 3 +-- > migration/savevm.c | 16 +++++--------- > 5 files changed, 28 insertions(+), 59 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 696762bc64..806b7b080b 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -285,7 +285,7 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque) > void migration_cancel(const Error *error) > { > if (error) { > - migrate_set_error(current_migration, error); > + migrate_report_err(error_copy(error)); > } > if (migrate_dirty_limit()) { > qmp_cancel_vcpu_dirty_limit(false, -1, NULL); > @@ -779,13 +779,6 @@ process_incoming_migration_co(void *opaque) > } > > if (ret < 0) { > - MigrationState *s = migrate_get_current(); > - > - if (migrate_has_error(s)) { > - WITH_QEMU_LOCK_GUARD(&s->error_mutex) { > - error_report_err(s->error); > - } > - } > error_report("load of migration failed: %s", strerror(-ret)); > goto fail; > } > @@ -1402,10 +1395,6 @@ static void migrate_fd_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); > @@ -1418,12 +1407,14 @@ static void migrate_fd_cleanup_bh(void *opaque) > migrate_fd_cleanup(opaque); > } > > -void migrate_set_error(MigrationState *s, const Error *error) > +void migrate_report_err(Error *error) > { > + MigrationState *s = migrate_get_current(); Avoid passing in *s looks ok. > QEMU_LOCK_GUARD(&s->error_mutex); > if (!s->error) { > s->error = error_copy(error); I think I get your point, but then looks like this error_copy() should be removed but forgotten? I remember I had an attempt to do similarly (but only transfer the ownership): https://lore.kernel.org/qemu-devel/20230829214235.69309-3-peterx@redhat.com/ However I gave up later and I forgot why. One reason could be that I hit a use-after-free, then found that well indeed leaking an Error is much better than debugging a UAF. So maybe we simply keep it like before? If you like such change, let's just be extremely caucious. > } > + error_report_err(error); The ideal case to me is we don't trigger an error_report() all over the place. We're slightly going backward from that regard, IMHO.. Ideally I think we shouldn't dump anything to stderr, but user should always query from qmp. Thanks,
On 24.04.24 22:40, Peter Xu wrote: > On Wed, Apr 24, 2024 at 08:42:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> 1. Most of callers want to free the error after call. Let's help them. >> >> 2. Some callers log the error, some not. We also have places where we >> log the stored error. Let's instead simply log every migration >> error. >> >> 3. Some callers have to retrieve current migration state only to pass >> it to migrate_set_error(). In the new helper let's get the state >> automatically. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> migration/migration.c | 48 ++++++++++++---------------------------- >> migration/migration.h | 2 +- >> migration/multifd.c | 18 ++++++--------- >> migration/postcopy-ram.c | 3 +-- >> migration/savevm.c | 16 +++++--------- >> 5 files changed, 28 insertions(+), 59 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 696762bc64..806b7b080b 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -285,7 +285,7 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque) >> void migration_cancel(const Error *error) >> { >> if (error) { >> - migrate_set_error(current_migration, error); >> + migrate_report_err(error_copy(error)); >> } >> if (migrate_dirty_limit()) { >> qmp_cancel_vcpu_dirty_limit(false, -1, NULL); >> @@ -779,13 +779,6 @@ process_incoming_migration_co(void *opaque) >> } >> >> if (ret < 0) { >> - MigrationState *s = migrate_get_current(); >> - >> - if (migrate_has_error(s)) { >> - WITH_QEMU_LOCK_GUARD(&s->error_mutex) { >> - error_report_err(s->error); >> - } >> - } >> error_report("load of migration failed: %s", strerror(-ret)); >> goto fail; >> } >> @@ -1402,10 +1395,6 @@ static void migrate_fd_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); >> @@ -1418,12 +1407,14 @@ static void migrate_fd_cleanup_bh(void *opaque) >> migrate_fd_cleanup(opaque); >> } >> >> -void migrate_set_error(MigrationState *s, const Error *error) >> +void migrate_report_err(Error *error) >> { >> + MigrationState *s = migrate_get_current(); > > Avoid passing in *s looks ok. > >> QEMU_LOCK_GUARD(&s->error_mutex); >> if (!s->error) { >> s->error = error_copy(error); > > I think I get your point, but then looks like this error_copy() should be > removed but forgotten? > > I remember I had an attempt to do similarly (but only transfer the > ownership): > > https://lore.kernel.org/qemu-devel/20230829214235.69309-3-peterx@redhat.com/ > > However I gave up later and I forgot why. One reason could be that I hit a > use-after-free, then found that well indeed leaking an Error is much better > than debugging a UAF. Hmm, yes I saw a leaked Error somewhere, and this patch should fix it. But may be I missed introducing this use-after-free again) > > So maybe we simply keep it like before? If you like such change, let's > just be extremely caucious. > >> } >> + error_report_err(error); > > The ideal case to me is we don't trigger an error_report() all over the > place. We're slightly going backward from that regard, IMHO.. > > Ideally I think we shouldn't dump anything to stderr, but user should > always query from qmp. > Sounds reasonable to me. I'm just unsure, if keep it like before, how should I correctly update logging to stderr in process_incoming_migration_co(). Could we just drop error reporting to stderr? Or should it be kept as is for the case when exit_on_error is true?
On Wed, Apr 24, 2024 at 10:50:14PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 24.04.24 22:40, Peter Xu wrote: > > On Wed, Apr 24, 2024 at 08:42:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > 1. Most of callers want to free the error after call. Let's help them. > > > > > > 2. Some callers log the error, some not. We also have places where we > > > log the stored error. Let's instead simply log every migration > > > error. > > > > > > 3. Some callers have to retrieve current migration state only to pass > > > it to migrate_set_error(). In the new helper let's get the state > > > automatically. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > --- > > > migration/migration.c | 48 ++++++++++++---------------------------- > > > migration/migration.h | 2 +- > > > migration/multifd.c | 18 ++++++--------- > > > migration/postcopy-ram.c | 3 +-- > > > migration/savevm.c | 16 +++++--------- > > > 5 files changed, 28 insertions(+), 59 deletions(-) > > > > > > diff --git a/migration/migration.c b/migration/migration.c > > > index 696762bc64..806b7b080b 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -285,7 +285,7 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque) > > > void migration_cancel(const Error *error) > > > { > > > if (error) { > > > - migrate_set_error(current_migration, error); > > > + migrate_report_err(error_copy(error)); > > > } > > > if (migrate_dirty_limit()) { > > > qmp_cancel_vcpu_dirty_limit(false, -1, NULL); > > > @@ -779,13 +779,6 @@ process_incoming_migration_co(void *opaque) > > > } > > > if (ret < 0) { > > > - MigrationState *s = migrate_get_current(); > > > - > > > - if (migrate_has_error(s)) { > > > - WITH_QEMU_LOCK_GUARD(&s->error_mutex) { > > > - error_report_err(s->error); > > > - } > > > - } > > > error_report("load of migration failed: %s", strerror(-ret)); > > > goto fail; > > > } > > > @@ -1402,10 +1395,6 @@ static void migrate_fd_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); > > > @@ -1418,12 +1407,14 @@ static void migrate_fd_cleanup_bh(void *opaque) > > > migrate_fd_cleanup(opaque); > > > } > > > -void migrate_set_error(MigrationState *s, const Error *error) > > > +void migrate_report_err(Error *error) > > > { > > > + MigrationState *s = migrate_get_current(); > > > > Avoid passing in *s looks ok. > > > > > QEMU_LOCK_GUARD(&s->error_mutex); > > > if (!s->error) { > > > s->error = error_copy(error); > > > > I think I get your point, but then looks like this error_copy() should be > > removed but forgotten? > > > > I remember I had an attempt to do similarly (but only transfer the > > ownership): > > > > https://lore.kernel.org/qemu-devel/20230829214235.69309-3-peterx@redhat.com/ > > > > However I gave up later and I forgot why. One reason could be that I hit a > > use-after-free, then found that well indeed leaking an Error is much better > > than debugging a UAF. > > Hmm, yes I saw a leaked Error somewhere, and this patch should fix it. But may be I missed introducing this use-after-free again) Ah do you mean you fixed a bug? If so please use a standalone patch for that and we'll need to copy stable. I did notice that in this series on patch looks like does more than one thing. It'll be helpful too for reviewers when patch can be split properly. > > > > > So maybe we simply keep it like before? If you like such change, let's > > just be extremely caucious. > > > > > } > > > + error_report_err(error); > > > > The ideal case to me is we don't trigger an error_report() all over the > > place. We're slightly going backward from that regard, IMHO.. > > > > Ideally I think we shouldn't dump anything to stderr, but user should > > always query from qmp. > > > > Sounds reasonable to me. I'm just unsure, if keep it like before, how > should I correctly update logging to stderr in > process_incoming_migration_co(). Could we just drop error reporting to > stderr? Or should it be kept as is for the case when exit_on_error is > true? I'm not sure I get the question, but I'll comment in patch 2 very soon, so we can move the discussion there. Thanks,
On 25.04.24 00:52, Peter Xu wrote: > On Wed, Apr 24, 2024 at 10:50:14PM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 24.04.24 22:40, Peter Xu wrote: >>> On Wed, Apr 24, 2024 at 08:42:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>>> 1. Most of callers want to free the error after call. Let's help them. >>>> >>>> 2. Some callers log the error, some not. We also have places where we >>>> log the stored error. Let's instead simply log every migration >>>> error. >>>> >>>> 3. Some callers have to retrieve current migration state only to pass >>>> it to migrate_set_error(). In the new helper let's get the state >>>> automatically. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>>> --- >>>> migration/migration.c | 48 ++++++++++++---------------------------- >>>> migration/migration.h | 2 +- >>>> migration/multifd.c | 18 ++++++--------- >>>> migration/postcopy-ram.c | 3 +-- >>>> migration/savevm.c | 16 +++++--------- >>>> 5 files changed, 28 insertions(+), 59 deletions(-) >>>> >>>> diff --git a/migration/migration.c b/migration/migration.c >>>> index 696762bc64..806b7b080b 100644 >>>> --- a/migration/migration.c >>>> +++ b/migration/migration.c >>>> @@ -285,7 +285,7 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque) >>>> void migration_cancel(const Error *error) >>>> { >>>> if (error) { >>>> - migrate_set_error(current_migration, error); >>>> + migrate_report_err(error_copy(error)); >>>> } >>>> if (migrate_dirty_limit()) { >>>> qmp_cancel_vcpu_dirty_limit(false, -1, NULL); >>>> @@ -779,13 +779,6 @@ process_incoming_migration_co(void *opaque) >>>> } >>>> if (ret < 0) { >>>> - MigrationState *s = migrate_get_current(); >>>> - >>>> - if (migrate_has_error(s)) { >>>> - WITH_QEMU_LOCK_GUARD(&s->error_mutex) { >>>> - error_report_err(s->error); >>>> - } >>>> - } >>>> error_report("load of migration failed: %s", strerror(-ret)); >>>> goto fail; >>>> } >>>> @@ -1402,10 +1395,6 @@ static void migrate_fd_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); >>>> @@ -1418,12 +1407,14 @@ static void migrate_fd_cleanup_bh(void *opaque) >>>> migrate_fd_cleanup(opaque); >>>> } >>>> -void migrate_set_error(MigrationState *s, const Error *error) >>>> +void migrate_report_err(Error *error) >>>> { >>>> + MigrationState *s = migrate_get_current(); >>> >>> Avoid passing in *s looks ok. >>> >>>> QEMU_LOCK_GUARD(&s->error_mutex); >>>> if (!s->error) { >>>> s->error = error_copy(error); >>> >>> I think I get your point, but then looks like this error_copy() should be >>> removed but forgotten? >>> >>> I remember I had an attempt to do similarly (but only transfer the >>> ownership): >>> >>> https://lore.kernel.org/qemu-devel/20230829214235.69309-3-peterx@redhat.com/ >>> >>> However I gave up later and I forgot why. One reason could be that I hit a >>> use-after-free, then found that well indeed leaking an Error is much better >>> than debugging a UAF. >> >> Hmm, yes I saw a leaked Error somewhere, and this patch should fix it. But may be I missed introducing this use-after-free again) > > Ah do you mean you fixed a bug? If so please use a standalone patch for > that and we'll need to copy stable. > Hmm, this hunk: @@ -1287,7 +1283,7 @@ static void multifd_recv_terminate_threads(Error *err) if (err) { MigrationState *s = migrate_get_current(); - migrate_set_error(s, err); + migrate_report_err(error_copy(err)); if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_ACTIVE) { migrate_set_state(&s->state, s->state, No, seems I don't fix bug, but exactly introduce use-after-free :). Will fix. > I did notice that in this series on patch looks like does more than one > thing. It'll be helpful too for reviewers when patch can be split > properly. Agree, OK > >> >>> >>> So maybe we simply keep it like before? If you like such change, let's >>> just be extremely caucious. >>> >>>> } >>>> + error_report_err(error); >>> >>> The ideal case to me is we don't trigger an error_report() all over the >>> place. We're slightly going backward from that regard, IMHO.. >>> >>> Ideally I think we shouldn't dump anything to stderr, but user should >>> always query from qmp. >>> >> >> Sounds reasonable to me. I'm just unsure, if keep it like before, how >> should I correctly update logging to stderr in >> process_incoming_migration_co(). Could we just drop error reporting to >> stderr? Or should it be kept as is for the case when exit_on_error is >> true? > > I'm not sure I get the question, but I'll comment in patch 2 very soon, so > we can move the discussion there. > > Thanks, >
diff --git a/migration/migration.c b/migration/migration.c index 696762bc64..806b7b080b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -285,7 +285,7 @@ void migration_bh_schedule(QEMUBHFunc *cb, void *opaque) void migration_cancel(const Error *error) { if (error) { - migrate_set_error(current_migration, error); + migrate_report_err(error_copy(error)); } if (migrate_dirty_limit()) { qmp_cancel_vcpu_dirty_limit(false, -1, NULL); @@ -779,13 +779,6 @@ process_incoming_migration_co(void *opaque) } if (ret < 0) { - MigrationState *s = migrate_get_current(); - - if (migrate_has_error(s)) { - WITH_QEMU_LOCK_GUARD(&s->error_mutex) { - error_report_err(s->error); - } - } error_report("load of migration failed: %s", strerror(-ret)); goto fail; } @@ -1402,10 +1395,6 @@ static void migrate_fd_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); @@ -1418,12 +1407,14 @@ static void migrate_fd_cleanup_bh(void *opaque) migrate_fd_cleanup(opaque); } -void migrate_set_error(MigrationState *s, const Error *error) +void migrate_report_err(Error *error) { + MigrationState *s = migrate_get_current(); QEMU_LOCK_GUARD(&s->error_mutex); if (!s->error) { s->error = error_copy(error); } + error_report_err(error); } bool migrate_has_error(MigrationState *s) @@ -1448,7 +1439,7 @@ static void migrate_fd_error(MigrationState *s, const Error *error) assert(s->to_dst_file == NULL); migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED); - migrate_set_error(s, error); + migrate_report_err(error_copy(error)); } static void migrate_fd_cancel(MigrationState *s) @@ -1863,8 +1854,7 @@ void qmp_migrate_pause(Error **errp) /* Tell the core migration that we're pausing */ error_setg(&error, "Postcopy migration is paused by the user"); - migrate_set_error(ms, error); - error_free(error); + migrate_report_err(error); qemu_mutex_lock(&ms->qemu_file_lock); if (ms->to_dst_file) { @@ -2413,8 +2403,7 @@ static void *source_return_path_thread(void *opaque) out: if (err) { - migrate_set_error(ms, err); - error_free(err); + migrate_report_err(err); trace_source_return_path_thread_bad_end(); } @@ -2842,12 +2831,10 @@ static void migration_completion(MigrationState *s) fail: if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_report_err(local_err); } else if (ret) { error_setg_errno(&local_err, -ret, "Error in migration completion"); - migrate_set_error(s, local_err); - error_free(local_err); + migrate_report_err(local_err); } migration_completion_failed(s, current_active_state); @@ -3070,8 +3057,7 @@ static MigThrError migration_detect_error(MigrationState *s) } if (local_error) { - migrate_set_error(s, local_error); - error_free(local_error); + migrate_report_err(local_error); } if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret) { @@ -3242,8 +3228,7 @@ static MigIterateState migration_iteration_run(MigrationState *s) if (!in_postcopy && must_precopy <= s->threshold_size && can_switchover && qatomic_read(&s->start_postcopy)) { if (postcopy_start(s, &local_err)) { - migrate_set_error(s, local_err); - error_report_err(local_err); + migrate_report_err(local_err); } return MIG_ITERATE_SKIP; } @@ -3487,8 +3472,7 @@ static void *migration_thread(void *opaque) * devices to unplug. This to preserve migration state transitions. */ if (ret) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_report_err(local_err); migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED); goto out; @@ -3613,8 +3597,7 @@ static void *bg_migration_thread(void *opaque) * devices to unplug. This to preserve migration state transitions. */ if (ret) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_report_err(local_err); migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED); goto fail_setup; @@ -3723,8 +3706,6 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) * It's normally done in migrate_fd_cleanup(), but call it here * explicitly. */ - error_report_err(error_copy(s->error)); - } else { migrate_fd_cleanup(s); } return; @@ -3794,9 +3775,8 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) return; fail: - migrate_set_error(s, local_err); + migrate_report_err(local_err); migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); - error_report_err(local_err); migrate_fd_cleanup(s); } diff --git a/migration/migration.h b/migration/migration.h index 8045e39c26..f9f436c33e 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -475,7 +475,7 @@ void migration_incoming_process(void); bool migration_has_all_channels(void); -void migrate_set_error(MigrationState *s, const Error *error); +void migrate_report_err(Error *error); bool migrate_has_error(MigrationState *s); void migrate_fd_connect(MigrationState *s, Error *error_in); diff --git a/migration/multifd.c b/migration/multifd.c index f317bff077..ce51744054 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -678,7 +678,7 @@ retry: } /* Multifd send side hit an error; remember it and prepare to quit */ -static void multifd_send_set_error(Error *err) +static void multifd_report_err(Error *err) { /* * We don't want to exit each threads twice. Depending on where @@ -692,7 +692,7 @@ static void multifd_send_set_error(Error *err) if (err) { MigrationState *s = migrate_get_current(); - migrate_set_error(s, err); + migrate_report_err(err); if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_PRE_SWITCHOVER || s->state == MIGRATION_STATUS_DEVICE || @@ -819,8 +819,7 @@ void multifd_send_shutdown(void) Error *local_err = NULL; if (!multifd_send_cleanup_channel(p, &local_err)) { - migrate_set_error(migrate_get_current(), local_err); - error_free(local_err); + migrate_report_err(local_err); } } @@ -998,9 +997,8 @@ out: if (ret) { assert(local_err); trace_multifd_send_error(p->id); - multifd_send_set_error(local_err); + multifd_report_err(local_err); multifd_send_kick_main(p); - error_free(local_err); } rcu_unregister_thread(); @@ -1123,14 +1121,13 @@ out: } trace_multifd_new_send_channel_async_error(p->id, local_err); - multifd_send_set_error(local_err); + multifd_report_err(local_err); /* * For error cases (TLS or non-TLS), IO channel is always freed here * rather than when cleanup multifd: since p->c is not set, multifd * cleanup code doesn't even know its existence. */ object_unref(OBJECT(ioc)); - error_free(local_err); } static bool multifd_new_send_channel_create(gpointer opaque, Error **errp) @@ -1214,8 +1211,7 @@ bool multifd_send_setup(void) } if (ret) { - migrate_set_error(s, local_err); - error_report_err(local_err); + migrate_report_err(local_err); migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED); return false; @@ -1287,7 +1283,7 @@ static void multifd_recv_terminate_threads(Error *err) if (err) { MigrationState *s = migrate_get_current(); - migrate_set_error(s, err); + migrate_report_err(error_copy(err)); if (s->state == MIGRATION_STATUS_SETUP || s->state == MIGRATION_STATUS_ACTIVE) { migrate_set_state(&s->state, s->state, diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index eccff499cb..dff220d529 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1626,8 +1626,7 @@ postcopy_preempt_send_channel_done(MigrationState *s, QIOChannel *ioc, Error *local_err) { if (local_err) { - migrate_set_error(s, local_err); - error_free(local_err); + migrate_report_err(local_err); } else { migration_ioc_register_yank(ioc); s->postcopy_qemufile_src = qemu_file_new_output(ioc); diff --git a/migration/savevm.c b/migration/savevm.c index 5d200cf42a..839957b5d7 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1103,14 +1103,12 @@ void qemu_savevm_send_open_return_path(QEMUFile *f) int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len) { uint32_t tmp; - MigrationState *ms = migrate_get_current(); Error *local_err = NULL; if (len > MAX_VM_CMD_PACKAGED_SIZE) { error_setg(&local_err, "%s: Unreasonably large packaged state: %zu", __func__, len); - migrate_set_error(ms, local_err); - error_report_err(local_err); + migrate_report_err(local_err); return -1; } @@ -1325,7 +1323,7 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp) if (se->vmsd && se->vmsd->early_setup) { ret = vmstate_save(f, se, ms->vmdesc, errp); if (ret) { - migrate_set_error(ms, *errp); + migrate_report_err(error_copy(*errp)); qemu_file_set_error(f, ret); break; } @@ -1553,8 +1551,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, ret = vmstate_save(f, se, vmdesc, &local_err); if (ret) { - migrate_set_error(ms, local_err); - error_report_err(local_err); + migrate_report_err(local_err); qemu_file_set_error(f, ret); return ret; } @@ -1571,8 +1568,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, if (ret) { error_setg(&local_err, "%s: bdrv_inactivate_all() failed (%d)", __func__, ret); - migrate_set_error(ms, local_err); - error_report_err(local_err); + migrate_report_err(local_err); qemu_file_set_error(f, ret); return ret; } @@ -1768,7 +1764,6 @@ void qemu_savevm_live_state(QEMUFile *f) int qemu_save_device_state(QEMUFile *f) { - MigrationState *ms = migrate_get_current(); Error *local_err = NULL; SaveStateEntry *se; @@ -1786,8 +1781,7 @@ int qemu_save_device_state(QEMUFile *f) } ret = vmstate_save(f, se, NULL, &local_err); if (ret) { - migrate_set_error(ms, local_err); - error_report_err(local_err); + migrate_report_err(local_err); return ret; } }
1. Most of callers want to free the error after call. Let's help them. 2. Some callers log the error, some not. We also have places where we log the stored error. Let's instead simply log every migration error. 3. Some callers have to retrieve current migration state only to pass it to migrate_set_error(). In the new helper let's get the state automatically. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- migration/migration.c | 48 ++++++++++++---------------------------- migration/migration.h | 2 +- migration/multifd.c | 18 ++++++--------- migration/postcopy-ram.c | 3 +-- migration/savevm.c | 16 +++++--------- 5 files changed, 28 insertions(+), 59 deletions(-)