Hi Dave,
On 6/25/19 12:18 PM, 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.
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 / dgilb...@redhat.com / Manchester, UK
>
