Patchwork [4/9] runstate_set(): Check for valid transitions

login
register
mail settings
Submitter Luiz Capitulino
Date Sept. 6, 2011, 1:14 p.m.
Message ID <1315314868-24770-5-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/113558/
State New
Headers show

Comments

Luiz Capitulino - Sept. 6, 2011, 1:14 p.m.
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(-)
Jan Kiszka - Sept. 6, 2011, 3:55 p.m.
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.

    case RSTATE_IO_ERROR:
    case RSTATE_PAUSED:
        switch (new_state) {
        case RSTATE_RUNNING:
            goto transition_ok;
        default:
            /* invalid transition */
            abort();
        }
        abort();

Jan
Luiz Capitulino - Sept. 6, 2011, 4:47 p.m.
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.

> 
>     case RSTATE_IO_ERROR:
>     case RSTATE_PAUSED:
>         switch (new_state) {
>         case RSTATE_RUNNING:
>             goto transition_ok;
>         default:
>             /* invalid transition */
>             abort();
>         }
>         abort();
> 
> Jan
>
Lluís Vilanova - Sept. 6, 2011, 5:42 p.m.
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:

    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
Luiz Capitulino - Sept. 9, 2011, 12:58 p.m.
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
>

Patch

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