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:
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
