qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] migration: Use RunState enum to save global sta


From: Dr. David Alan Gilbert
Subject: Re: [Qemu-devel] [PATCH] migration: Use RunState enum to save global state pre migrate
Date: Tue, 25 Jun 2019 11:18:00 +0100
User-agent: Mutt/1.12.0 (2019-05-25)

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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]