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

[Philippe Mathieu-Daudé]

Hi Dave,

On 6/25/19 12:18 PM, 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.

Do you mind adding this information in a comment around GlobalState?

Thanks,

Phil.

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