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

[Peter Xu <peterx@redhat.com>]

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

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/.

Thanks,

>  }
>  
>  bool migration_in_setup(MigrationState *s)
> @@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>       * spice needs to trigger a transition now
>       */
>      ms->postcopy_after_devices = true;
> -    migration_call_notifiers(ms);
> +    if (migration_call_notifiers(ms)) {
> +        goto fail;
> +    }
>  
>      migration_downtime_end(ms);
>  
> @@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>          rate_limit = migrate_max_bandwidth();
>  
>          /* Notify before starting migration thread */
> -        migration_call_notifiers(s);
> +        if (migration_call_notifiers(s)) {
> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +            migrate_fd_cleanup(s);
> +            return;
> +        }
>      }
>  
>      migration_rate_set(rate_limit);
> -- 
> 1.8.3.1
> 

-- 
Peter Xu