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

[Wei Yang]

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.

>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