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

[Luiz Capitulino <lcapitulino@redhat.com>]

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
>