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

[Wei Yang <richardw.yang@linux.intel.com>]

On Fri, Aug 23, 2019 at 11:21:50AM -0500, Eric Blake wrote:
>On 8/19/19 9:08 AM, Wei Yang wrote:
>> On Mon, Aug 19, 2019 at 12:26:32PM +0100, Dr. David Alan Gilbert wrote:
>>> * Wei Yang (richardw.yang@linux.intel.com) wrote:
>
>Typo in the subject line: migrtion should be migration
>
>>>> 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
>
>Those are compiler-defined macros, so you have to consult the compiler
>documentation to see if they state what happens when invoked on an enum
>type.  You also have to check whether our macro
>typeof_strip_qual(enum_type) produces 'int' or something else.
>
>C99 doesn't specify _Atomic at all (which is why we handrolled our own
>atomic.h built on top of compiler primitives, instead of using
><stdatomic.h>).  But reading C11, I see that 6.7.2.4 states that
>_Atomic(type) is okay except for:
>
>"The type name in an atomic type specifier shall not refer to an array
>type, a function type, an atomic type, or a qualified type."
>
>which does NOT preclude the use of _Atomic(enum_type), so presumably
>compilers have to be prepared to handle an atomic enum type.  Still,
>it's rather shaky ground if you can't prove compilers handle it correctly.
>

Sounds this is a dark area for all those compilers. I would keep the code
untouched now.

Thanks

>
>> 
>> 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.
>
>What C99 says is rather weak; you really want to be basing your
>decisions on atomics based on C11 or later.
>
>
>-- 
>Eric Blake, Principal Software Engineer
>Red Hat, Inc.           +1-919-301-3226
>Virtualization:  qemu.org | libvirt.org
>




-- 
Wei Yang
Help you, Help me