On Tue, 06 Sep 2011 19:42:15 +0200 Lluís Vilanova <vilan...@ac.upc.edu> wrote:
> Luiz Capitulino writes: > > > On Tue, 06 Sep 2011 17:55:12 +0200 > > Jan Kiszka <jan.kis...@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 <lcapitul...@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 >