Re: [Qemu-devel] [PATCH] migration: Use RunState enum to save global state pre migrate

[Maxiwell S. Garcia]

On Tue, Jun 25, 2019 at 11:18:00AM +0100, Dr. David Alan Gilbert wrote:
> * Maxiwell S. Garcia (address@hidden) 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 <address@hidden>
> 
> 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 / address@hidden / Manchester, UK
>