Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> On Mon, Aug 19, 2019 at 12:26:32PM +0100, Dr. David Alan Gilbert wrote:
> >* Wei Yang (address@hidden) 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.

Hmm OK, but I'd need you to test n some 32bit and other comp=ilers etc;
make sure it works on 32bit, clang, gcc, 32bit ARM etc - because
otherwise I'm going to have to worry about checking all those.

Dave

> >then we also might have to make the old_state and new_state
> >variables match.
> >
> 
> You are right.
> >Dave
> >
> >> Signed-off-by: Wei Yang <address@hidden>
> >> ---
> >>  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 / address@hidden / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK