On Tue, Jun 25, 2019 at 11:18:00AM +0100, Dr. David Alan Gilbert wrote: > * Maxiwell S. Garcia (maxiw...@linux.ibm.com) wrote: > > The GlobalState struct has two confusing fields: > > - uint8_t runstate[100] > > - RunState state > > > > The first field saves the 'current_run_state' from vl.c file before > > migrate. The second field is filled in the post load func using the > > 'runstate' value. So, this commit renames the 'runstate' to > > 'state_pre_migrate' and use the same type used by 'state' and > > 'current_run_state' variables. > > > > Signed-off-by: Maxiwell S. Garcia <maxiw...@linux.ibm.com> > > Hi, > Thanks for the patch. > > Unfortunately this wont work for a few different reasons: > > a) 'RunState' is an enum whose order and encoding is not specified - > to keep migration compatibility the wire format must be stable. > The textual version is more stable. > > b) It's also too late to change it, because existing migration streams > send the textual Runstate; this change breaks migration > compatibility from/to existing qemu's. >
It makes sense. What do you think about adding a comment to explain it (as suggest Philippe) and change the 'runstate' to 'state_stored' (or 'state_pre_migrate') to improve the legibility? Thanks, > Dave > > > --- > > include/sysemu/sysemu.h | 2 +- > > migration/global_state.c | 65 ++++++---------------------------------- > > vl.c | 11 ++----- > > 3 files changed, 12 insertions(+), 66 deletions(-) > > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > > index 61579ae71e..483b536c4f 100644 > > --- a/include/sysemu/sysemu.h > > +++ b/include/sysemu/sysemu.h > > @@ -23,7 +23,7 @@ bool runstate_check(RunState state); > > void runstate_set(RunState new_state); > > int runstate_is_running(void); > > bool runstate_needs_reset(void); > > -bool runstate_store(char *str, size_t size); > > +RunState runstate_get(void); > > typedef struct vm_change_state_entry VMChangeStateEntry; > > typedef void VMChangeStateHandler(void *opaque, int running, RunState > > state); > > > > diff --git a/migration/global_state.c b/migration/global_state.c > > index 2c8c447239..b49b99f3a1 100644 > > --- a/migration/global_state.c > > +++ b/migration/global_state.c > > @@ -20,8 +20,7 @@ > > #include "trace.h" > > > > typedef struct { > > - uint32_t size; > > - uint8_t runstate[100]; > > + RunState state_pre_migrate; > > RunState state; > > bool received; > > } GlobalState; > > @@ -30,21 +29,14 @@ static GlobalState global_state; > > > > int global_state_store(void) > > { > > - if (!runstate_store((char *)global_state.runstate, > > - sizeof(global_state.runstate))) { > > - error_report("runstate name too big: %s", global_state.runstate); > > - trace_migrate_state_too_big(); > > - return -EINVAL; > > - } > > + global_state.state_pre_migrate = runstate_get(); > > + > > return 0; > > } > > > > void global_state_store_running(void) > > { > > - const char *state = RunState_str(RUN_STATE_RUNNING); > > - assert(strlen(state) < sizeof(global_state.runstate)); > > - strncpy((char *)global_state.runstate, > > - state, sizeof(global_state.runstate)); > > + global_state.state_pre_migrate = RUN_STATE_RUNNING; > > } > > > > bool global_state_received(void) > > @@ -60,7 +52,6 @@ RunState global_state_get_runstate(void) > > static bool global_state_needed(void *opaque) > > { > > GlobalState *s = opaque; > > - char *runstate = (char *)s->runstate; > > > > /* If it is not optional, it is mandatory */ > > > > @@ -70,8 +61,8 @@ static bool global_state_needed(void *opaque) > > > > /* If state is running or paused, it is not needed */ > > > > - if (strcmp(runstate, "running") == 0 || > > - strcmp(runstate, "paused") == 0) { > > + if (s->state_pre_migrate == RUN_STATE_RUNNING || > > + s->state_pre_migrate == RUN_STATE_PAUSED) { > > return false; > > } > > > > @@ -82,45 +73,10 @@ static bool global_state_needed(void *opaque) > > static int global_state_post_load(void *opaque, int version_id) > > { > > GlobalState *s = opaque; > > - Error *local_err = NULL; > > - int r; > > - char *runstate = (char *)s->runstate; > > - > > s->received = true; > > - trace_migrate_global_state_post_load(runstate); > > - > > - if (strnlen((char *)s->runstate, > > - sizeof(s->runstate)) == sizeof(s->runstate)) { > > - /* > > - * This condition should never happen during migration, because > > - * all runstate names are shorter than 100 bytes (the size of > > - * s->runstate). However, a malicious stream could overflow > > - * the qapi_enum_parse() call, so we force the last character > > - * to a NUL byte. > > - */ > > - s->runstate[sizeof(s->runstate) - 1] = '\0'; > > - } > > - r = qapi_enum_parse(&RunState_lookup, runstate, -1, &local_err); > > - > > - if (r == -1) { > > - if (local_err) { > > - error_report_err(local_err); > > - } > > - return -EINVAL; > > - } > > - s->state = r; > > - > > - return 0; > > -} > > - > > -static int global_state_pre_save(void *opaque) > > -{ > > - GlobalState *s = opaque; > > - > > - trace_migrate_global_state_pre_save((char *)s->runstate); > > - s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1; > > - assert(s->size <= sizeof(s->runstate)); > > + s->state = s->state_pre_migrate; > > > > + trace_migrate_global_state_post_load(RunState_str(s->state)); > > return 0; > > } > > > > @@ -129,11 +85,9 @@ static const VMStateDescription vmstate_globalstate = { > > .version_id = 1, > > .minimum_version_id = 1, > > .post_load = global_state_post_load, > > - .pre_save = global_state_pre_save, > > .needed = global_state_needed, > > .fields = (VMStateField[]) { > > - VMSTATE_UINT32(size, GlobalState), > > - VMSTATE_BUFFER(runstate, GlobalState), > > + VMSTATE_UINT32(state_pre_migrate, GlobalState), > > VMSTATE_END_OF_LIST() > > }, > > }; > > @@ -141,7 +95,6 @@ static const VMStateDescription vmstate_globalstate = { > > void register_global_state(void) > > { > > /* We would use it independently that we receive it */ > > - strcpy((char *)&global_state.runstate, ""); > > global_state.received = false; > > vmstate_register(NULL, 0, &vmstate_globalstate, &global_state); > > } > > diff --git a/vl.c b/vl.c > > index 99a56b5556..2b15d68d60 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -680,16 +680,9 @@ bool runstate_check(RunState state) > > return current_run_state == state; > > } > > > > -bool runstate_store(char *str, size_t size) > > +RunState runstate_get(void) > > { > > - const char *state = RunState_str(current_run_state); > > - size_t len = strlen(state) + 1; > > - > > - if (len > size) { > > - return false; > > - } > > - memcpy(str, state, len); > > - return true; > > + return current_run_state; > > } > > > > static void runstate_init(void) > > -- > > 2.20.1 > > > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK >