On Wed, Jan 10, 2024 at 01:08:41PM -0500, Steven Sistare wrote:
> On 1/10/2024 2:18 AM, Peter Xu wrote:
> > On Wed, Dec 13, 2023 at 10:11:32AM -0800, Steve Sistare wrote:
> >> After calling notifiers, check if an error has been reported via
> >> migrate_set_error, and halt the migration.
> >>
> >> None of the notifiers call migrate_set_error at this time, so no
> >> functional change.
> >>
> >> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
> >> ---
> >> include/migration/misc.h | 2 +-
> >> migration/migration.c | 26 ++++++++++++++++++++++----
> >> 2 files changed, 23 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/migration/misc.h b/include/migration/misc.h
> >> index 901d117..231d7e4 100644
> >> --- a/include/migration/misc.h
> >> +++ b/include/migration/misc.h
> >> @@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
> >> void migration_add_notifier(Notifier *notify,
> >> void (*func)(Notifier *notifier, void *data));
> >> void migration_remove_notifier(Notifier *notify);
> >> -void migration_call_notifiers(MigrationState *s);
> >> +int migration_call_notifiers(MigrationState *s);
> >> bool migration_in_setup(MigrationState *);
> >> bool migration_has_finished(MigrationState *);
> >> bool migration_has_failed(MigrationState *);
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index d5bfe70..29a9a92 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state,
> >> int new_state)
> >>
> >> static void migrate_fd_cleanup(MigrationState *s)
> >> {
> >> + bool already_failed;
> >> +
> >> qemu_bh_delete(s->cleanup_bh);
> >> s->cleanup_bh = NULL;
> >>
> >> @@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
> >> MIGRATION_STATUS_CANCELLED);
> >> }
> >>
> >> + already_failed = migration_has_failed(s);
> >> + if (migration_call_notifiers(s)) {
> >> + if (!already_failed) {
> >> + migrate_set_state(&s->state, s->state,
> >> MIGRATION_STATUS_FAILED);
> >> + /* Notify again to recover from this late failure. */
> >> + migration_call_notifiers(s);
> >> + }
> >> + }
> >> +
> >> if (s->error) {
> >> /* It is used on info migrate. We can't free it */
> >> error_report_err(error_copy(s->error));
> >> }
> >> - migration_call_notifiers(s);
> >> +
> >> block_cleanup_parameters();
> >> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> >> }
> >> @@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
> >> }
> >> }
> >>
> >> -void migration_call_notifiers(MigrationState *s)
> >> +int migration_call_notifiers(MigrationState *s)
> >> {
> >> notifier_list_notify(&migration_state_notifiers, s);
> >> + return (s->error != NULL);
> >
> > Exporting more migration_*() functions is pretty ugly to me..
>
> I assume you mean migrate_set_error(), which is currently only called from
> migration/*.c code.
>
> Instead, we could define a new function migrate_set_notifier_error(), defined
> in the new file migration/notifier.h, so we clearly limit the migration
> functions which can be called from notifiers. (Its implementation just calls
> migrate_set_error)
Fundementally this allows another .c to change one more field of
MigrationState (which is ->error) and I still want to avoid it.
I just replied in the other thread, but now with all these in mind I think
I still prefer not passing in MigrationState* at all. It's already kind of
abused due to migrate_get_current(), and IMHO it's healthier to limit its
usage to minimum to cover the core of migration states for migration/ use
only.
Shrinking or even stop exporting migrate_get_current() is another more
challenging task, but now what we can do is stop enlarging the direct use
of MigrationState*.
>
> > Would it be better to pass in "Error** errp" into each notifiers? That may
> > need an open coded notifier_list_notify(), breaking the loop if "*errp".
> >
> > And the notifier API currently only support one arg.. maybe we should
> > implement the notifiers ourselves, ideally passing in "(int state, Error
> > **errp)" instead of "(MigrationState *s)".
> >
> > Ideally with that MigrationState* shouldn't be visible outside migration/.
>
> I will regret saying this because of the amount of (mechanical) code change
> involved,
> but the cleanest solution is:
:)
>
> * Pass errp to:
> notifier_with_return_list_notify(NotifierWithReturnList *list, void *data,
> Error *errp)
> * Pass errp to the NotifierWithReturn notifier:
> int (*notify)(NotifierWithReturn *notifier, void *data, Error **errp);
> * Delete the errp member from struct PostcopyNotifyData and pass errp to the
> notifier function
> Ditto for PrecopyNotifyData.
> * Convert all migration notifiers to NotifierWithReturn
Would you mind changing MigrationState* into an event just like postcopy?
We don't need to use migration_has_failed() etc., afaict three events
should be enough for the existing four users, exactly like what postcopy
does:
- MIG_EVENT_PRECOPY_SETUP
- MIG_EVENT_PRECOPY_DONE
- MIG_EVENT_PRECOPY_FAILED
Merging postcopy will be indeed the cleanest. I'm okay if you want to
leave that for later, but if you'd do that together I'd appreciate that.
Thanks,
--
Peter Xu
