On Mon, Aug 19, 2019 at 12:26:32PM +0100, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> No functional change. Add default case to fix warning. > >I think the problem with this is that migrate_set_state uses an >atomic_cmpxchg and so we have to be careful that the type we use >is compatible with that. >MigrationStatus is an enum and I think compilers are allowed to >choose the types of that; so I'm not sure we're guaranteed >that an enum is always OK for the atomic_cmpxchg, and if it is
Took a look into the definition of atomic_cmpxchg, which finally calls * __atomic_compare_exchange_n for c++11 * __sync_val_compare_and_swap Both of them take two pointers to compare and exchange its content. Per C99 standard, http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf, it mentioned: Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined, but shall be capable of representing the values of all the members of the enumeration. Based on this, I think atomic_cmpxchg should work fine with enum. >then we also might have to make the old_state and new_state >variables match. > You are right. >Dave > >> Signed-off-by: Wei Yang <richardw.y...@linux.intel.com> >> --- >> migration/migration.c | 8 +++++++- >> migration/migration.h | 6 +++--- >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index 2865ae3fa9..0fd2364961 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -946,6 +946,8 @@ static void fill_source_migration_info(MigrationInfo >> *info) >> case MIGRATION_STATUS_CANCELLED: >> info->has_status = true; >> break; >> + default: >> + return; >> } >> info->status = s->state; >> } >> @@ -1054,6 +1056,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; >> } >> @@ -1446,7 +1450,7 @@ 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, int old_state, int new_state) >> { >> assert(new_state < MIGRATION_STATUS__MAX); >> if (atomic_cmpxchg(state, old_state, new_state) == old_state) { >> @@ -1683,6 +1687,8 @@ bool migration_is_idle(void) >> return false; >> case MIGRATION_STATUS__MAX: >> g_assert_not_reached(); >> + default: >> + g_assert_not_reached(); >> } >> >> return false; >> diff --git a/migration/migration.h b/migration/migration.h >> index 5e8f09c6db..418ee00053 100644 >> --- a/migration/migration.h >> +++ b/migration/migration.h >> @@ -65,7 +65,7 @@ struct MigrationIncomingState { >> >> QEMUBH *bh; >> >> - int state; >> + MigrationStatus state; >> >> bool have_colo_incoming_thread; >> QemuThread colo_incoming_thread; >> @@ -151,7 +151,7 @@ struct MigrationState >> /* params from 'migrate-set-parameters' */ >> MigrationParameters parameters; >> >> - int state; >> + MigrationStatus state; >> >> /* State related to return path */ >> struct { >> @@ -234,7 +234,7 @@ struct MigrationState >> bool decompress_error_check; >> }; >> >> -void migrate_set_state(int *state, int old_state, int new_state); >> +void migrate_set_state(MigrationStatus *state, int old_state, int >> new_state); >> >> void migration_fd_process_incoming(QEMUFile *f); >> void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp); >> -- >> 2.19.1 >> >-- >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Wei Yang Help you, Help me
