| Message ID | 1315314868-24770-5-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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 >
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; }
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(-)