[v1] migration: Introduce POSTCOPY_DEVICE state

[PATCH 2/4] migration: Accept MigrationStatus in migration_has_failed()

Posted by Juraj Marcin 4 months, 3 weeks ago

From: Juraj Marcin <jmarcin@redhat.com>

This allows to reuse the helper also with MigrationIncomingState.

Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
---
 migration/migration.c | 8 ++++----
 migration/migration.h | 2 +-
 migration/multifd.c   | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 54dac3db88..2c0b3a7229 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1542,7 +1542,7 @@ static void migration_cleanup(MigrationState *s)
         /* 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 :
+    type = migration_has_failed(s->state) ? MIG_EVENT_PRECOPY_FAILED :
                                      MIG_EVENT_PRECOPY_DONE;
     migration_call_notifiers(s, type, NULL);
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1700,10 +1700,10 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
     return ret;
 }
 
-bool migration_has_failed(MigrationState *s)
+bool migration_has_failed(MigrationStatus state)
 {
-    return (s->state == MIGRATION_STATUS_CANCELLED ||
-            s->state == MIGRATION_STATUS_FAILED);
+    return (state == MIGRATION_STATUS_CANCELLED ||
+            state == MIGRATION_STATUS_FAILED);
 }
 
 bool migration_in_postcopy(void)
diff --git a/migration/migration.h b/migration/migration.h
index 01329bf824..2c2331f40d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -535,7 +535,7 @@ bool migration_is_blocked(Error **errp);
 bool migration_in_postcopy(void);
 bool migration_postcopy_is_alive(MigrationStatus state);
 MigrationState *migrate_get_current(void);
-bool migration_has_failed(MigrationState *);
+bool migration_has_failed(MigrationStatus state);
 bool migrate_mode_is_cpr(MigrationState *);
 
 uint64_t ram_get_total_transferred_pages(void);
diff --git a/migration/multifd.c b/migration/multifd.c
index b255778855..c569f91f2c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -568,7 +568,7 @@ void multifd_send_shutdown(void)
              * already failed. If the migration succeeded, errors are
              * not expected but there's no need to kill the source.
              */
-            if (local_err && !migration_has_failed(migrate_get_current())) {
+            if (local_err && !migration_has_failed(migrate_get_current()->state)) {
                 warn_report(
                     "multifd_send_%d: Failed to terminate TLS connection: %s",
                     p->id, error_get_pretty(local_err));
-- 
2.51.0

Re: [PATCH 2/4] migration: Accept MigrationStatus in migration_has_failed()

Posted by Peter Xu 4 months, 3 weeks ago

On Mon, Sep 15, 2025 at 01:59:13PM +0200, Juraj Marcin wrote:
> From: Juraj Marcin <jmarcin@redhat.com>
> 
> This allows to reuse the helper also with MigrationIncomingState.

I get the point, but just to mention that this helper doesn't really change
much on incoming side on simplifying the code or function-wise, because we
don't have CANCELLING/CANCELLED state on deste QEMU.. which is definitely
not obvious.. :(

So:

  migration_has_failed(incoming->state)

Is exactly the same as:

  incoming->state == MIGRATION_STATUS_FAILED

Except it will make src start to pass in s->state.. which is slightly more
awkward.

Maybe we keep the MIGRATION_STATUS_FAILED check in your next patch, and
drop this one for now until it grows more than FAILED on dest?

> 
> Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> ---
>  migration/migration.c | 8 ++++----
>  migration/migration.h | 2 +-
>  migration/multifd.c   | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 54dac3db88..2c0b3a7229 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1542,7 +1542,7 @@ static void migration_cleanup(MigrationState *s)
>          /* 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 :
> +    type = migration_has_failed(s->state) ? MIG_EVENT_PRECOPY_FAILED :
>                                       MIG_EVENT_PRECOPY_DONE;
>      migration_call_notifiers(s, type, NULL);
>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1700,10 +1700,10 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>      return ret;
>  }
>  
> -bool migration_has_failed(MigrationState *s)
> +bool migration_has_failed(MigrationStatus state)
>  {
> -    return (s->state == MIGRATION_STATUS_CANCELLED ||
> -            s->state == MIGRATION_STATUS_FAILED);
> +    return (state == MIGRATION_STATUS_CANCELLED ||
> +            state == MIGRATION_STATUS_FAILED);
>  }
>  
>  bool migration_in_postcopy(void)
> diff --git a/migration/migration.h b/migration/migration.h
> index 01329bf824..2c2331f40d 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -535,7 +535,7 @@ bool migration_is_blocked(Error **errp);
>  bool migration_in_postcopy(void);
>  bool migration_postcopy_is_alive(MigrationStatus state);
>  MigrationState *migrate_get_current(void);
> -bool migration_has_failed(MigrationState *);
> +bool migration_has_failed(MigrationStatus state);
>  bool migrate_mode_is_cpr(MigrationState *);
>  
>  uint64_t ram_get_total_transferred_pages(void);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b255778855..c569f91f2c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -568,7 +568,7 @@ void multifd_send_shutdown(void)
>               * already failed. If the migration succeeded, errors are
>               * not expected but there's no need to kill the source.
>               */
> -            if (local_err && !migration_has_failed(migrate_get_current())) {
> +            if (local_err && !migration_has_failed(migrate_get_current()->state)) {
>                  warn_report(
>                      "multifd_send_%d: Failed to terminate TLS connection: %s",
>                      p->id, error_get_pretty(local_err));
> -- 
> 2.51.0
> 

-- 
Peter Xu

Re: [PATCH 2/4] migration: Accept MigrationStatus in migration_has_failed()

Posted by Juraj Marcin 4 months, 2 weeks ago

Hi Peter,

On 2025-09-19 10:57, Peter Xu wrote:
> On Mon, Sep 15, 2025 at 01:59:13PM +0200, Juraj Marcin wrote:
> > From: Juraj Marcin <jmarcin@redhat.com>
> > 
> > This allows to reuse the helper also with MigrationIncomingState.
> 
> I get the point, but just to mention that this helper doesn't really change
> much on incoming side on simplifying the code or function-wise, because we
> don't have CANCELLING/CANCELLED state on deste QEMU.. which is definitely
> not obvious.. :(
> 
> So:
> 
>   migration_has_failed(incoming->state)
> 
> Is exactly the same as:
> 
>   incoming->state == MIGRATION_STATUS_FAILED
> 
> Except it will make src start to pass in s->state.. which is slightly more
> awkward.
> 
> Maybe we keep the MIGRATION_STATUS_FAILED check in your next patch, and
> drop this one for now until it grows more than FAILED on dest?

Sure, that sounds like a good solution.

> 
> > 
> > Signed-off-by: Juraj Marcin <jmarcin@redhat.com>
> > ---
> >  migration/migration.c | 8 ++++----
> >  migration/migration.h | 2 +-
> >  migration/multifd.c   | 2 +-
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 54dac3db88..2c0b3a7229 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1542,7 +1542,7 @@ static void migration_cleanup(MigrationState *s)
> >          /* 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 :
> > +    type = migration_has_failed(s->state) ? MIG_EVENT_PRECOPY_FAILED :
> >                                       MIG_EVENT_PRECOPY_DONE;
> >      migration_call_notifiers(s, type, NULL);
> >      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> > @@ -1700,10 +1700,10 @@ int migration_call_notifiers(MigrationState *s, MigrationEventType type,
> >      return ret;
> >  }
> >  
> > -bool migration_has_failed(MigrationState *s)
> > +bool migration_has_failed(MigrationStatus state)
> >  {
> > -    return (s->state == MIGRATION_STATUS_CANCELLED ||
> > -            s->state == MIGRATION_STATUS_FAILED);
> > +    return (state == MIGRATION_STATUS_CANCELLED ||
> > +            state == MIGRATION_STATUS_FAILED);
> >  }
> >  
> >  bool migration_in_postcopy(void)
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 01329bf824..2c2331f40d 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -535,7 +535,7 @@ bool migration_is_blocked(Error **errp);
> >  bool migration_in_postcopy(void);
> >  bool migration_postcopy_is_alive(MigrationStatus state);
> >  MigrationState *migrate_get_current(void);
> > -bool migration_has_failed(MigrationState *);
> > +bool migration_has_failed(MigrationStatus state);
> >  bool migrate_mode_is_cpr(MigrationState *);
> >  
> >  uint64_t ram_get_total_transferred_pages(void);
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index b255778855..c569f91f2c 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -568,7 +568,7 @@ void multifd_send_shutdown(void)
> >               * already failed. If the migration succeeded, errors are
> >               * not expected but there's no need to kill the source.
> >               */
> > -            if (local_err && !migration_has_failed(migrate_get_current())) {
> > +            if (local_err && !migration_has_failed(migrate_get_current()->state)) {
> >                  warn_report(
> >                      "multifd_send_%d: Failed to terminate TLS connection: %s",
> >                      p->id, error_get_pretty(local_err));
> > -- 
> > 2.51.0
> > 
> 
> -- 
> Peter Xu
>

[PATCH 1/4] migration: Do not try to start VM if disk activation fails
[PATCH 2/4] migration: Accept MigrationStatus in migration_has_failed()
[PATCH 3/4] migration: Refactor incoming cleanup into migration_incoming_finish()
[PATCH 4/4] migration: Introduce POSTCOPY_DEVICE state