From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58957) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R20fZ-0002hu-M9 for qemu-devel@nongnu.org; Fri, 09 Sep 2011 08:58:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R20fX-0002Dm-Hk for qemu-devel@nongnu.org; Fri, 09 Sep 2011 08:58:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20322) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R20fW-0002Co-V7 for qemu-devel@nongnu.org; Fri, 09 Sep 2011 08:58:55 -0400 Date: Fri, 9 Sep 2011 09:58:45 -0300 From: Luiz Capitulino Message-ID: <20110909095845.0b1496f0@doriath> In-Reply-To: <87r53td0ug.fsf@ginnungagap.bsc.es> References: <1315314868-24770-1-git-send-email-lcapitulino@redhat.com> <1315314868-24770-5-git-send-email-lcapitulino@redhat.com> <4E664260.3010303@siemens.com> <20110906134718.4fb8a130@doriath> <87r53td0ug.fsf@ginnungagap.bsc.es> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/9] runstate_set(): Check for valid transitions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?B?TGx17XM=?= Vilanova Cc: "kwolf@redhat.com" , "aliguori@us.ibm.com" , Jan Kiszka , "armbru@redhat.com" , "qemu-devel@nongnu.org" , "amit.shah@redhat.com" On Tue, 06 Sep 2011 19:42:15 +0200 Llu=EDs Vilanova wrote: > Luiz Capitulino writes: >=20 > > On Tue, 06 Sep 2011 17:55:12 +0200 > > Jan Kiszka wrote: >=20 > >> 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. > >> >=20 > >> > Checking and testing all valid transitions wasn't trivial, chances > >> > are this will need broader testing to become more stable. > >> >=20 > >> > Signed-off-by: Luiz Capitulino > >> > --- > >> > vl.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++++++- > >> > 1 files changed, 148 insertions(+), 1 deletions(-) > >> >=20 > >> > 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 =3D=3D state; > >> > } > >> > =20 > >> > +/* 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 =3D new_state; > >> > } > >> > =20 > >>=20 > >> I haven't looked at the transitions yet, but just to make the function > >> smaller: you could fold identical blocks together, e.g. >=20 > > I thought about doing that but I fear it's error-prone: you extend > > RSTATE_PAUSED and forget about RSTATE_IO_ERROR. >=20 > > I think it's better to have different things separated, that's, each st= ate > > has its own switch statement. >=20 > You could also use a state transition matrix instead: Looks like a good idea. >=20 > typedef enum { > RSTATE_NO_STATE, > RSTATE_RUNNING, > RSTATE_IN_MIGRATE, > ... > RSTATE_COUNT > } RunState; > =20 > typedef struct > { > RunState from; > RunState to; > } RunStateTransition; > =20 >=20 > // relevant transition definition here =20 > static RunStateTransition trans_def[] =3D > { > {RSTATE_NO_STATE, RSTATE_RUNNING}, > {RSTATE_NO_STATE, RSTATE_IN_MIGRATE}, > ... > {RSTATE_COUNT, RSTATE_COUNT}, > }; > =20 > static bool trans_matrix[RSTATE_COUNT][RSTATE_COUNT]; >=20 > // call at system initialization =20 > void > runstate_init(void) > { > bzero(trans_matrix, sizeof(trans_matrix)); > =20 > RunStateTransition *trans; > for (trans =3D &trans_def[0]; trans->from !=3D RSTATE_COUNT; tran= s++) { > trans_matrix[trans->from][trans->to] =3D true; > } > } I think I prefer a static init. > =20 > void runstate_set(RunState new_state) > { > if (unlikely(current_run_state >=3D 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 =3D new_state; > } >=20 > 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). >=20 >=20 > Lluis >=20