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

[Lluís Vilanova]

Luiz Capitulino writes:

> On Tue, 06 Sep 2011 17:55:12 +0200
> Jan Kiszka <address@hidden> 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 <address@hidden>
>> > ---
>> >  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:

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

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth