From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46075) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0yo5-0007Yy-Ab for qemu-devel@nongnu.org; Tue, 06 Sep 2011 12:47:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R0ynz-0001kt-DM for qemu-devel@nongnu.org; Tue, 06 Sep 2011 12:47:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60436) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0yny-0001ko-Qf for qemu-devel@nongnu.org; Tue, 06 Sep 2011 12:47:23 -0400 Date: Tue, 6 Sep 2011 13:47:18 -0300 From: Luiz Capitulino Message-ID: <20110906134718.4fb8a130@doriath> In-Reply-To: <4E664260.3010303@siemens.com> References: <1315314868-24770-1-git-send-email-lcapitulino@redhat.com> <1315314868-24770-5-git-send-email-lcapitulino@redhat.com> <4E664260.3010303@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: Jan Kiszka Cc: "kwolf@redhat.com" , "amit.shah@redhat.com" , "aliguori@us.ibm.com" , "qemu-devel@nongnu.org" , "armbru@redhat.com" On Tue, 06 Sep 2011 17:55:12 +0200 Jan Kiszka 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 > > --- > > 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 >