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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 06 Sep 2011 19:42:15 +0200
Lluís Vilanova <vilanova@ac.upc.edu> wrote:

> Luiz Capitulino writes:
> 
> > On Tue, 06 Sep 2011 17:55:12 +0200
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
> >> On 2011-09-06 15:14, Luiz Capitulino 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.
> >> > 
> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > ---
> >> >  vl.c |  149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >> >  1 files changed, 148 insertions(+), 1 deletions(-)
> >> > 
> >> > diff --git a/vl.c b/vl.c
> >> > index 9926d2a..fe3628a 100644
> >> > --- a/vl.c
> >> > +++ b/vl.c
> >> > @@ -332,9 +332,156 @@ bool runstate_check(RunState state)
> >> >      return current_run_state == state;
> >> >  }
> >> >  
> >> > +/* This function will abort() on invalid state transitions */
> >> >  void runstate_set(RunState new_state)
> >> >  {
> >> > -    assert(new_state < RSTATE_MAX);
> >> > +    switch (current_run_state) {
> >> > +    case RSTATE_NO_STATE:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +        case RSTATE_IN_MIGRATE:
> >> > +        case RSTATE_PRE_LAUNCH:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_DEBUG:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_IN_MIGRATE:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +        case RSTATE_PRE_LAUNCH:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_PANICKED:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_PAUSED:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_IO_ERROR:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_PAUSED:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_POST_MIGRATE:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_PRE_LAUNCH:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +        case RSTATE_POST_MIGRATE:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_PRE_MIGRATE:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +        case RSTATE_POST_MIGRATE:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_RESTORE:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_RUNNING:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_DEBUG:
> >> > +        case RSTATE_PANICKED:
> >> > +        case RSTATE_IO_ERROR:
> >> > +        case RSTATE_PAUSED:
> >> > +        case RSTATE_PRE_MIGRATE:
> >> > +        case RSTATE_RESTORE:
> >> > +        case RSTATE_SAVEVM:
> >> > +        case RSTATE_SHUTDOWN:
> >> > +        case RSTATE_WATCHDOG:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_SAVEVM:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_SHUTDOWN:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_PAUSED:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    case RSTATE_WATCHDOG:
> >> > +        switch (new_state) {
> >> > +        case RSTATE_RUNNING:
> >> > +            goto transition_ok;
> >> > +        default:
> >> > +            /* invalid transition */
> >> > +            abort();
> >> > +        }
> >> > +        abort();
> >> > +    default:
> >> > +        fprintf(stderr, "current run state is invalid: %s\n",
> >> > +                runstate_as_string());
> >> > +        abort();
> >> > +    }
> >> > +
> >> > +transition_ok:
> >> >      current_run_state = new_state;
> >> >  }
> >> >  
> >> 
> >> I haven't looked at the transitions yet, but just to make the function
> >> smaller: you could fold identical blocks together, e.g.
> 
> > I thought about doing that but I fear it's error-prone: you extend
> > RSTATE_PAUSED and forget about RSTATE_IO_ERROR.
> 
> > I think it's better to have different things separated, that's, each state
> > has its own switch statement.
> 
> You could also use a state transition matrix instead:

Looks like a good idea.

> 
>     typedef enum {
>         RSTATE_NO_STATE,
>         RSTATE_RUNNING,
>         RSTATE_IN_MIGRATE,
>         ...
>         RSTATE_COUNT
>     }  RunState;
>      
>     typedef struct
>     {
>         RunState from;
>         RunState to;
>     } RunStateTransition;
>      
> 
>     // relevant transition definition here     
>     static RunStateTransition trans_def[] =
>     {
>         {RSTATE_NO_STATE, RSTATE_RUNNING},
>         {RSTATE_NO_STATE, RSTATE_IN_MIGRATE},
>         ...
>         {RSTATE_COUNT, RSTATE_COUNT},
>     };
>      
>     static bool trans_matrix[RSTATE_COUNT][RSTATE_COUNT];
> 
>     // call at system initialization     
>     void
>     runstate_init(void)
>     {
>         bzero(trans_matrix, sizeof(trans_matrix));
>      
>         RunStateTransition *trans;
>         for (trans = &trans_def[0]; trans->from != RSTATE_COUNT; trans++) {
>             trans_matrix[trans->from][trans->to] = true;
>         }
>     }

I think I prefer a static init.

>      
>     void runstate_set(RunState new_state)
>     {
>         if (unlikely(current_run_state >= RSTATE_COUNT)) {
>             fprintf(stderr, "current run state is invalid: %s\n",
>                     runstate_as_string());
>             abort();
>         }
>         if (unlikely(!trans_matrix[current_run_state][new_state])) {
>             fprintf(stderr, "invalid run state transition\n");
>             abort();
>         }
>         current_run_state = new_state;
>     }
> 
> I think it's easier to read the state machine from 'trans_def', and it
> can be easily extended to include other fields in the future (like
> flags, callbacks or whatever).
> 
> 
> Lluis
>