Re: [PATCH V1 2/3] migration: notifier error reporting

[Peter Xu <peterx@redhat.com>]

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.sistare@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