| Message ID | 20240612144228.1179240-4-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | migration: New postcopy state, and some cleanups | expand |
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)
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<===
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 --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(); }
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(-)