diff mbox series

[3/4] migration: Use MigrationStatus instead of int

Message ID 20240612144228.1179240-4-peterx@redhat.com
State New
Headers show
Series migration: New postcopy state, and some cleanups | expand

Commit Message

Peter Xu June 12, 2024, 2:42 p.m. UTC
QEMU uses "int" in most cases even if it stores MigrationStatus.  I don't
know why, so let's try to do that right and see what blows up..

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h |  9 +++++----
 migration/migration.c | 13 +++++++------
 2 files changed, 12 insertions(+), 10 deletions(-)

Comments

Peter Xu June 12, 2024, 3:03 p.m. UTC | #1
On Wed, Jun 12, 2024 at 10:42:27AM -0400, Peter Xu wrote:
> @@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
>      }
>  }
>  
> -bool migration_postcopy_is_alive(int state)
> +bool migration_postcopy_is_alive(MigrationStatus state)
>  {
>      switch (state) {
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> @@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
>      case MIGRATION_STATUS_DEVICE:
>      case MIGRATION_STATUS_WAIT_UNPLUG:
>          return false;
> -    case MIGRATION_STATUS__MAX:
> +    default:
>          g_assert_not_reached();
>      }

This survives the tests, but I just found that it's risky, as it's not
covering all the states..

I'll squash below when I send v2 instead.

===8<===
From 1fc42c76294044c0ccca8841e482472486de5459 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Wed, 12 Jun 2024 10:57:26 -0400
Subject: [PATCH] fixup! migration: Use MigrationStatus instead of int

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 9475dce7dc..706cee1b69 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1637,20 +1637,9 @@ bool migration_is_idle(void)
     case MIGRATION_STATUS_COMPLETED:
     case MIGRATION_STATUS_FAILED:
         return true;
-    case MIGRATION_STATUS_SETUP:
-    case MIGRATION_STATUS_CANCELLING:
-    case MIGRATION_STATUS_ACTIVE:
-    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-    case MIGRATION_STATUS_COLO:
-    case MIGRATION_STATUS_PRE_SWITCHOVER:
-    case MIGRATION_STATUS_DEVICE:
-    case MIGRATION_STATUS_WAIT_UNPLUG:
-        return false;
     default:
-        g_assert_not_reached();
+        return false;
     }
-
-    return false;
 }
 
 bool migration_is_active(void)
Fabiano Rosas June 13, 2024, 2:59 p.m. UTC | #2
Peter Xu <peterx@redhat.com> writes:

> On Wed, Jun 12, 2024 at 10:42:27AM -0400, Peter Xu wrote:
>> @@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
>>      }
>>  }
>>  
>> -bool migration_postcopy_is_alive(int state)
>> +bool migration_postcopy_is_alive(MigrationStatus state)
>>  {
>>      switch (state) {
>>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>> @@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
>>      case MIGRATION_STATUS_DEVICE:
>>      case MIGRATION_STATUS_WAIT_UNPLUG:
>>          return false;
>> -    case MIGRATION_STATUS__MAX:
>> +    default:
>>          g_assert_not_reached();
>>      }
>
> This survives the tests, but I just found that it's risky, as it's not
> covering all the states..
>
> I'll squash below when I send v2 instead.
>
> ===8<===
> From 1fc42c76294044c0ccca8841e482472486de5459 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Wed, 12 Jun 2024 10:57:26 -0400
> Subject: [PATCH] fixup! migration: Use MigrationStatus instead of int
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 9475dce7dc..706cee1b69 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1637,20 +1637,9 @@ bool migration_is_idle(void)
>      case MIGRATION_STATUS_COMPLETED:
>      case MIGRATION_STATUS_FAILED:
>          return true;
> -    case MIGRATION_STATUS_SETUP:
> -    case MIGRATION_STATUS_CANCELLING:
> -    case MIGRATION_STATUS_ACTIVE:
> -    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> -    case MIGRATION_STATUS_COLO:
> -    case MIGRATION_STATUS_PRE_SWITCHOVER:
> -    case MIGRATION_STATUS_DEVICE:
> -    case MIGRATION_STATUS_WAIT_UNPLUG:
> -        return false;
>      default:
> -        g_assert_not_reached();
> +        return false;
>      }

Unrelated, but should we consider POSTCOPY_PAUSED as idle?

> -
> -    return false;
>  }
>  
>  bool migration_is_active(void)
> -- 
> 2.45.0
>
> ===8<===
Peter Xu June 13, 2024, 3:58 p.m. UTC | #3
On Thu, Jun 13, 2024 at 11:59:20AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Jun 12, 2024 at 10:42:27AM -0400, Peter Xu wrote:
> >> @@ -1544,7 +1545,7 @@ bool migration_in_postcopy(void)
> >>      }
> >>  }
> >>  
> >> -bool migration_postcopy_is_alive(int state)
> >> +bool migration_postcopy_is_alive(MigrationStatus state)
> >>  {
> >>      switch (state) {
> >>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >> @@ -1598,7 +1599,7 @@ bool migration_is_idle(void)
> >>      case MIGRATION_STATUS_DEVICE:
> >>      case MIGRATION_STATUS_WAIT_UNPLUG:
> >>          return false;
> >> -    case MIGRATION_STATUS__MAX:
> >> +    default:
> >>          g_assert_not_reached();
> >>      }
> >
> > This survives the tests, but I just found that it's risky, as it's not
> > covering all the states..
> >
> > I'll squash below when I send v2 instead.
> >
> > ===8<===
> > From 1fc42c76294044c0ccca8841e482472486de5459 Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@redhat.com>
> > Date: Wed, 12 Jun 2024 10:57:26 -0400
> > Subject: [PATCH] fixup! migration: Use MigrationStatus instead of int
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 9475dce7dc..706cee1b69 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1637,20 +1637,9 @@ bool migration_is_idle(void)
> >      case MIGRATION_STATUS_COMPLETED:
> >      case MIGRATION_STATUS_FAILED:
> >          return true;
> > -    case MIGRATION_STATUS_SETUP:
> > -    case MIGRATION_STATUS_CANCELLING:
> > -    case MIGRATION_STATUS_ACTIVE:
> > -    case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > -    case MIGRATION_STATUS_COLO:
> > -    case MIGRATION_STATUS_PRE_SWITCHOVER:
> > -    case MIGRATION_STATUS_DEVICE:
> > -    case MIGRATION_STATUS_WAIT_UNPLUG:
> > -        return false;
> >      default:
> > -        g_assert_not_reached();
> > +        return false;
> >      }
> 
> Unrelated, but should we consider POSTCOPY_PAUSED as idle?

I think not; idle should be about "whether a migration is in progress" in
verbose, IMHO.

E.g. see migrate_add_blocker_modes() -> is_busy(), otherwise we'll allow
changing migration blockers when postcopy is paused.

PS: offtopic, but.. "is_busy" may deserve some better name in the future..
diff mbox series

Patch

diff --git a/migration/migration.h b/migration/migration.h
index 6af01362d4..38aa1402d5 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -160,7 +160,7 @@  struct MigrationIncomingState {
     /* PostCopyFD's for external userfaultfds & handlers of shared memory */
     GArray   *postcopy_remote_fds;
 
-    int state;
+    MigrationStatus state;
 
     /*
      * The incoming migration coroutine, non-NULL during qemu_loadvm_state().
@@ -301,7 +301,7 @@  struct MigrationState {
     /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
-    int state;
+    MigrationStatus state;
 
     /* State related to return path */
     struct {
@@ -459,7 +459,8 @@  struct MigrationState {
     bool rdma_migration;
 };
 
-void migrate_set_state(int *state, int old_state, int new_state);
+void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
+                       MigrationStatus new_state);
 
 void migration_fd_process_incoming(QEMUFile *f);
 void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
@@ -479,7 +480,7 @@  int migrate_init(MigrationState *s, Error **errp);
 bool migration_is_blocked(Error **errp);
 /* True if outgoing migration has entered postcopy phase */
 bool migration_in_postcopy(void);
-bool migration_postcopy_is_alive(int state);
+bool migration_postcopy_is_alive(MigrationStatus state);
 MigrationState *migrate_get_current(void);
 bool migration_has_failed(MigrationState *);
 bool migrate_mode_is_cpr(MigrationState *);
diff --git a/migration/migration.c b/migration/migration.c
index d41e00ed4c..bfbd657035 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -390,7 +390,7 @@  void migration_incoming_state_destroy(void)
     yank_unregister_instance(MIGRATION_YANK_INSTANCE);
 }
 
-static void migrate_generate_event(int new_state)
+static void migrate_generate_event(MigrationStatus new_state)
 {
     if (migrate_events()) {
         qapi_event_send_migration(new_state);
@@ -1273,8 +1273,6 @@  static void fill_destination_migration_info(MigrationInfo *info)
     }
 
     switch (mis->state) {
-    case MIGRATION_STATUS_NONE:
-        return;
     case MIGRATION_STATUS_SETUP:
     case MIGRATION_STATUS_CANCELLING:
     case MIGRATION_STATUS_CANCELLED:
@@ -1290,6 +1288,8 @@  static void fill_destination_migration_info(MigrationInfo *info)
         info->has_status = true;
         fill_destination_postcopy_migration_info(info);
         break;
+    default:
+        return;
     }
     info->status = mis->state;
 
@@ -1337,7 +1337,8 @@  void qmp_migrate_start_postcopy(Error **errp)
 
 /* shared migration helpers */
 
-void migrate_set_state(int *state, int old_state, int new_state)
+void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
+                       MigrationStatus new_state)
 {
     assert(new_state < MIGRATION_STATUS__MAX);
     if (qatomic_cmpxchg(state, old_state, new_state) == old_state) {
@@ -1544,7 +1545,7 @@  bool migration_in_postcopy(void)
     }
 }
 
-bool migration_postcopy_is_alive(int state)
+bool migration_postcopy_is_alive(MigrationStatus state)
 {
     switch (state) {
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
@@ -1598,7 +1599,7 @@  bool migration_is_idle(void)
     case MIGRATION_STATUS_DEVICE:
     case MIGRATION_STATUS_WAIT_UNPLUG:
         return false;
-    case MIGRATION_STATUS__MAX:
+    default:
         g_assert_not_reached();
     }