Re: [Qemu-devel] [PATCH 4/9] runstate_set(): Check for valid transitions

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 14 Sep 2011 19:55:25 +0000
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Wed, Sep 14, 2011 at 3:06 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Fri,  9 Sep 2011 17:25:41 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >
> >> This commit could have been folded with the previous one, however
> >> doing it separately will allow for easy bisect and revert if needed.
> >>
> >> Checking and testing all valid transitions wasn't trivial, chances
> >> are this will need broader testing to become more stable.
> >>
> >> This is a transition table as suggested by Lluís Vilanova.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >
> > Would be nice to get a reviewed-by for this patch, as it wasn't trivial
> > to get it right and tested...
> >
> >> ---
> >>  sysemu.h |    1 +
> >>  vl.c     |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 74 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/sysemu.h b/sysemu.h
> >> index 19088aa..a01ddac 100644
> >> --- a/sysemu.h
> >> +++ b/sysemu.h
> >> @@ -36,6 +36,7 @@ extern uint8_t qemu_uuid[];
> >>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
> >>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> >>
> >> +void runstate_init(void);
> >>  bool runstate_check(RunState state);
> >>  void runstate_set(RunState new_state);
> >>  typedef struct vm_change_state_entry VMChangeStateEntry;
> >> diff --git a/vl.c b/vl.c
> >> index 9926d2a..4a8edc7 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -327,14 +327,84 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> >>
> >>  static RunState current_run_state = RSTATE_NO_STATE;
> >>
> >> +typedef struct {
> >> +    RunState from;
> >> +    RunState to;
> >> +} RunStateTransition;
> >> +
> >> +static const RunStateTransition runstate_transitions_def[] = {
> >> +    /*     from      ->     to      */
> >> +    { RSTATE_NO_STATE, RSTATE_RUNNING },
> >> +    { RSTATE_NO_STATE, RSTATE_IN_MIGRATE },
> >> +    { RSTATE_NO_STATE, RSTATE_PRE_LAUNCH },
> >> +
> >> +    { RSTATE_DEBUG, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_IN_MIGRATE, RSTATE_RUNNING },
> >> +    { RSTATE_IN_MIGRATE, RSTATE_PRE_LAUNCH },
> >> +
> >> +    { RSTATE_PANICKED, RSTATE_PAUSED },
> >> +
> >> +    { RSTATE_IO_ERROR, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_PAUSED, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_POST_MIGRATE, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_PRE_LAUNCH, RSTATE_RUNNING },
> >> +    { RSTATE_PRE_LAUNCH, RSTATE_POST_MIGRATE },
> >> +
> >> +    { RSTATE_PRE_MIGRATE, RSTATE_RUNNING },
> >> +    { RSTATE_PRE_MIGRATE, RSTATE_POST_MIGRATE },
> >> +
> >> +    { RSTATE_RESTORE, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_RUNNING, RSTATE_DEBUG },
> >> +    { RSTATE_RUNNING, RSTATE_PANICKED },
> >> +    { RSTATE_RUNNING, RSTATE_IO_ERROR },
> >> +    { RSTATE_RUNNING, RSTATE_PAUSED },
> >> +    { RSTATE_RUNNING, RSTATE_PRE_MIGRATE },
> >> +    { RSTATE_RUNNING, RSTATE_RESTORE },
> >> +    { RSTATE_RUNNING, RSTATE_SAVEVM },
> >> +    { RSTATE_RUNNING, RSTATE_SHUTDOWN },
> >> +    { RSTATE_RUNNING, RSTATE_WATCHDOG },
> >> +
> >> +    { RSTATE_SAVEVM, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_SHUTDOWN, RSTATE_PAUSED },
> >> +
> >> +    { RSTATE_WATCHDOG, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_MAX, RSTATE_MAX },
> 
> I don't see a state which requires a system_reset to recover, which I
> think was the original motivation. Do I miss something?

That's done by patch 7/9 Monitor/QMP: Don't allow cont on bad VM state.

> Otherwise the patch looks fine.

Thanks!

> 
> >> +};
> >> +
> >> +static bool runstate_valid_transitions[RSTATE_MAX][RSTATE_MAX];
> >> +
> >>  bool runstate_check(RunState state)
> >>  {
> >>      return current_run_state == state;
> >>  }
> >>
> >> +void runstate_init(void)
> >> +{
> >> +    const RunStateTransition *p;
> >> +
> >> +    memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
> >> +
> >> +    for (p = &runstate_transitions_def[0]; p->from != RSTATE_MAX; p++) {
> >> +        runstate_valid_transitions[p->from][p->to] = true;
> >> +    }
> >> +}
> >> +
> >> +/* This function will abort() on invalid state transitions */
> >>  void runstate_set(RunState new_state)
> >>  {
> >> -    assert(new_state < RSTATE_MAX);
> >> +    if (new_state >= RSTATE_MAX ||
> >> +        !runstate_valid_transitions[current_run_state][new_state]) {
> >> +        fprintf(stderr, "invalid runstate transition\n");
> >> +        abort();
> >> +    }
> >> +
> >>      current_run_state = new_state;
> >>  }
> >>
> >> @@ -2218,6 +2288,8 @@ int main(int argc, char **argv, char **envp)
> >>
> >>      g_mem_set_vtable(&mem_trace);
> >>
> >> +    runstate_init();
> >> +
> >>      init_clocks();
> >>
> >>      qemu_cache_utils_init(envp);
> >
> >
> >
>