Re: [PATCH V7 05/12] migration: propagate suspended runstate

[Fabiano Rosas]

Steve Sistare <steven.sistare@oracle.com> writes:

> If the outgoing machine was previously suspended, propagate that to the
> incoming side via global_state, so a subsequent vm_start restores the
> suspended state.  To maintain backward and forward compatibility, reclaim
> some space from the runstate member.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  migration/global_state.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/migration/global_state.c b/migration/global_state.c
> index 4e2a9d8..d4f61a1 100644
> --- a/migration/global_state.c
> +++ b/migration/global_state.c
> @@ -22,7 +22,16 @@
>  
>  typedef struct {
>      uint32_t size;
> -    uint8_t runstate[100];
> +
> +    /*
> +     * runstate was 100 bytes, zero padded, but we trimmed it to add a
> +     * few fields and maintain backwards compatibility.
> +     */
> +    uint8_t runstate[32];
> +    uint8_t has_vm_was_suspended;
> +    uint8_t vm_was_suspended;
> +    uint8_t unused[66];
> +
>      RunState state;
>      bool received;
>  } GlobalState;
> @@ -35,6 +44,10 @@ static void global_state_do_store(RunState state)
>      assert(strlen(state_str) < sizeof(global_state.runstate));
>      strpadcpy((char *)global_state.runstate, sizeof(global_state.runstate),
>                state_str, '\0');
> +    global_state.has_vm_was_suspended = true;
> +    global_state.vm_was_suspended = vm_get_suspended();
> +
> +    memset(global_state.unused, 0, sizeof(global_state.unused));
>  }
>  
>  void global_state_store(void)
> @@ -68,6 +81,12 @@ static bool global_state_needed(void *opaque)
>          return true;
>      }
>  
> +    /* If the suspended state must be remembered, it is needed */
> +
> +    if (vm_get_suspended()) {
> +        return true;
> +    }
> +
>      /* If state is running or paused, it is not needed */
>  
>      if (strcmp(runstate, "running") == 0 ||
> @@ -85,6 +104,7 @@ static int global_state_post_load(void *opaque, int 
> version_id)
>      Error *local_err = NULL;
>      int r;
>      char *runstate = (char *)s->runstate;
> +    bool vm_was_suspended = s->has_vm_was_suspended && s->vm_was_suspended;

Why do you need has_vm_was_suspended? If they're always read like this,
then one flag should do, no?

>  
>      s->received = true;
>      trace_migrate_global_state_post_load(runstate);
> @@ -93,7 +113,7 @@ static int global_state_post_load(void *opaque, int 
> version_id)
>                  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
> +         * all runstate names are shorter than 32 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.
> @@ -110,6 +130,14 @@ static int global_state_post_load(void *opaque, int 
> version_id)
>      }
>      s->state = r;
>  
> +    /*
> +     * global_state is saved on the outgoing side before forcing a stopped
> +     * state, so it may have saved state=suspended and vm_was_suspended=0.
> +     * Now we are in a paused state, and when we later call vm_start, it must
> +     * restore the suspended state, so we must set vm_was_suspended=1 here.
> +     */
> +    vm_set_suspended(vm_was_suspended || r == RUN_STATE_SUSPENDED);
> +
>      return 0;
>  }
>  
> @@ -134,6 +162,9 @@ static const VMStateDescription vmstate_globalstate = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(size, GlobalState),
>          VMSTATE_BUFFER(runstate, GlobalState),
> +        VMSTATE_UINT8(has_vm_was_suspended, GlobalState),
> +        VMSTATE_UINT8(vm_was_suspended, GlobalState),
> +        VMSTATE_BUFFER(unused, GlobalState),
>          VMSTATE_END_OF_LIST()
>      },
>  };