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

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

[Wei Yang]

No functional change. Add default case to fix warning.

Signed-off-by: Wei Yang <richardw.yang@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

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

[Wei Yang]

On Fri, Jun 21, 2019 at 10:27:39PM +0800, Wei Yang wrote:
>No functional change. Add default case to fix warning.
>

Hi, David & Juan

Do you like this?

>Signed-off-by: Wei Yang <richardw.yang@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

-- 
Wei Yang
Help you, Help me

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

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Fri, Jun 21, 2019 at 10:27:39PM +0800, Wei Yang wrote:
> >No functional change. Add default case to fix warning.
> >
> 
> Hi, David & Juan
> 
> Do you like this?

See other reply; but you are making patches a bit faster than I can
review them!

Dave

> >Signed-off-by: Wei Yang <richardw.yang@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
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Wei Yang]

On Mon, Aug 19, 2019 at 12:26:55PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> On Fri, Jun 21, 2019 at 10:27:39PM +0800, Wei Yang wrote:
>> >No functional change. Add default case to fix warning.
>> >
>> 
>> Hi, David & Juan
>> 
>> Do you like this?
>
>See other reply; but you are making patches a bit faster than I can
>review them!
>

Sorry for that, my fault.

>Dave
>
>> >Signed-off-by: Wei Yang <richardw.yang@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
>> 
>> -- 
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Mon, Aug 19, 2019 at 12:26:55PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> On Fri, Jun 21, 2019 at 10:27:39PM +0800, Wei Yang wrote:
>> >No functional change. Add default case to fix warning.
>> >
>> 
>> Hi, David & Juan
>> 
>> Do you like this?
>
>See other reply; but you are making patches a bit faster than I can
>review them!

Can I post v2 with new_state changed to use MigrationStatus?

Or when you prefer me to send a v2?

>
>Dave
-- 
Wei Yang
Help you, Help me

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

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@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
then we also might have to make the old_state and new_state
variables match.

Dave

> Signed-off-by: Wei Yang <richardw.yang@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 / dgilbert@redhat.com / Manchester, UK

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 (richardw.yang@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.yang@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 / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

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

[Dr. David Alan Gilbert]

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

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 <richardw.yang@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 / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Wei Yang (richard.weiyang@gmail.com) wrote:
> > On Mon, Aug 19, 2019 at 12:26:32PM +0100, Dr. David Alan Gilbert wrote:
> > >* Wei Yang (richardw.yang@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.
> 
> 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.

Actually since QEMU is defined to be C99, the test is to check with some
C99 compilers; eblake assures me that in theory C11 should be OK,
but C99 and our atomics are more of an interesting question.

Dave
> 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 <richardw.yang@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 / dgilbert@redhat.com / Manchester, UK
> > 
> > -- 
> > Wei Yang
> > Help you, Help me
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eric Blake]

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

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

[Wei Yang]

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