Re: [PATCH V3 10/16] migration: split qmp_migrate

[Peter Xu <peterx@redhat.com>]

On Fri, Nov 01, 2024 at 06:47:49AM -0700, Steve Sistare wrote:
> Split qmp_migrate into start and finish functions.  Finish will be
> called asynchronously in a subsequent patch, but for now, call it
> immediately.  No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  migration/migration.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 6dc7c09..86b3f39 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1521,6 +1521,7 @@ static void migrate_fd_error(MigrationState *s, const Error *error)
>  static void migrate_fd_cancel(MigrationState *s)
>  {
>      int old_state ;
> +    bool setup = (s->state == MIGRATION_STATUS_SETUP);
>  
>      trace_migrate_fd_cancel();
>  
> @@ -1565,6 +1566,15 @@ static void migrate_fd_cancel(MigrationState *s)
>              s->block_inactive = false;
>          }
>      }
> +
> +    /*
> +     * If qmp_migrate_finish has not been called, then there is no path that
> +     * will complete the cancellation.  Do it now.
> +     */
> +    if (setup && !s->to_dst_file) {
> +        migrate_set_state(&s->state, s->state, MIGRATION_STATUS_CANCELLED);
> +        vm_resume(s->vm_old_state);
> +    }

Hmm.. this doesn't look like the right place to put this change.. as this
patch logically should bring no functional change if it's only about a new
helper split an existing function.

Meanwhile, this change also doesn't yet tell where does a vm_resume() came
from.. I'm really not sure whether this is correct at all, consider someone
does QMP "stop", migrate then quickly cancel it.  I suppose it may
accidentally resume the VM which it shouldn't.

Not to mention checking "setup" early, and unconditionally modify the state
here no matter what it is (can it be things like FAILED now, then
overwritten by a CANCELLED)?  But I'd confess that's not the problem of
this patch, but that migration state machine is currently still racy.. afaiu.

>  }
>  
>  void migration_add_notifier_mode(NotifierWithReturn *notify,
> @@ -2072,6 +2082,9 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>      return true;
>  }
>  
> +static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> +                               Error **errp);
> +
>  void qmp_migrate(const char *uri, bool has_channels,
>                   MigrationChannelList *channels, bool has_detach, bool detach,
>                   bool has_resume, bool resume, Error **errp)
> @@ -2118,6 +2131,20 @@ void qmp_migrate(const char *uri, bool has_channels,
>          return;
>      }
>  
> +    qmp_migrate_finish(addr, resume_requested, errp);
> +
> +    if (local_err) {
> +        migrate_fd_error(s, local_err);
> +        error_propagate(errp, local_err);
> +    }

I don't see when local_err will be set at all until here.. maybe you meant
*errp, but then maybe we should drop local_err and use ERRP_GUARD().

> +}
> +
> +static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> +                               Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +    Error *local_err = NULL;
> +
>      if (!resume_requested) {
>          if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
>              return;
> -- 
> 1.8.3.1
> 

-- 
Peter Xu