* [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus @ 2019-06-21 14:27 Wei Yang 2019-08-19 3:29 ` Wei Yang 2019-08-19 11:26 ` Dr. David Alan Gilbert 0 siblings, 2 replies; 11+ messages in thread From: Wei Yang @ 2019-06-21 14:27 UTC (permalink / raw) To: qemu-devel; +Cc: Wei Yang, dgilbert, quintela 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-06-21 14:27 [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus Wei Yang @ 2019-08-19 3:29 ` Wei Yang 2019-08-19 11:26 ` Dr. David Alan Gilbert 2019-08-19 11:26 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 11+ messages in thread From: Wei Yang @ 2019-08-19 3:29 UTC (permalink / raw) To: Wei Yang; +Cc: qemu-devel, dgilbert, quintela 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-19 3:29 ` Wei Yang @ 2019-08-19 11:26 ` Dr. David Alan Gilbert 2019-08-19 13:32 ` Wei Yang 2019-08-21 8:51 ` Wei Yang 0 siblings, 2 replies; 11+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-19 11:26 UTC (permalink / raw) To: Wei Yang; +Cc: qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-19 11:26 ` Dr. David Alan Gilbert @ 2019-08-19 13:32 ` Wei Yang 2019-08-21 8:51 ` Wei Yang 1 sibling, 0 replies; 11+ messages in thread From: Wei Yang @ 2019-08-19 13:32 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: quintela, Wei Yang, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-19 11:26 ` Dr. David Alan Gilbert 2019-08-19 13:32 ` Wei Yang @ 2019-08-21 8:51 ` Wei Yang 1 sibling, 0 replies; 11+ messages in thread From: Wei Yang @ 2019-08-21 8:51 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: quintela, Wei Yang, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-06-21 14:27 [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus Wei Yang 2019-08-19 3:29 ` Wei Yang @ 2019-08-19 11:26 ` Dr. David Alan Gilbert 2019-08-19 14:08 ` Wei Yang 1 sibling, 1 reply; 11+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-19 11:26 UTC (permalink / raw) To: Wei Yang; +Cc: qemu-devel, quintela * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-19 11:26 ` Dr. David Alan Gilbert @ 2019-08-19 14:08 ` Wei Yang 2019-08-23 15:29 ` Dr. David Alan Gilbert 2019-08-23 16:21 ` Eric Blake 0 siblings, 2 replies; 11+ messages in thread From: Wei Yang @ 2019-08-19 14:08 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: quintela, Wei Yang, qemu-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-19 14:08 ` Wei Yang @ 2019-08-23 15:29 ` Dr. David Alan Gilbert 2019-08-23 16:18 ` Dr. David Alan Gilbert 2019-08-23 16:21 ` Eric Blake 1 sibling, 1 reply; 11+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-23 15:29 UTC (permalink / raw) To: Wei Yang; +Cc: quintela, Wei Yang, qemu-devel * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-23 15:29 ` Dr. David Alan Gilbert @ 2019-08-23 16:18 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 11+ messages in thread From: Dr. David Alan Gilbert @ 2019-08-23 16:18 UTC (permalink / raw) To: Wei Yang; +Cc: quintela, Wei Yang, qemu-devel * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-19 14:08 ` Wei Yang 2019-08-23 15:29 ` Dr. David Alan Gilbert @ 2019-08-23 16:21 ` Eric Blake 2019-08-23 23:49 ` Wei Yang 1 sibling, 1 reply; 11+ messages in thread From: Eric Blake @ 2019-08-23 16:21 UTC (permalink / raw) To: Wei Yang, Dr. David Alan Gilbert; +Cc: qemu-devel, Wei Yang, quintela [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus 2019-08-23 16:21 ` Eric Blake @ 2019-08-23 23:49 ` Wei Yang 0 siblings, 0 replies; 11+ messages in thread From: Wei Yang @ 2019-08-23 23:49 UTC (permalink / raw) To: Eric Blake Cc: qemu-devel, Wei Yang, quintela, Dr. David Alan Gilbert, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-24 0:06 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-21 14:27 [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus Wei Yang 2019-08-19 3:29 ` Wei Yang 2019-08-19 11:26 ` Dr. David Alan Gilbert 2019-08-19 13:32 ` Wei Yang 2019-08-21 8:51 ` Wei Yang 2019-08-19 11:26 ` Dr. David Alan Gilbert 2019-08-19 14:08 ` Wei Yang 2019-08-23 15:29 ` Dr. David Alan Gilbert 2019-08-23 16:18 ` Dr. David Alan Gilbert 2019-08-23 16:21 ` Eric Blake 2019-08-23 23:49 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).