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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2430 bytes --]

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.


> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]