Patchwork [4/9] runstate_set(): Check for valid transitions

login
register
mail settings
Submitter Luiz Capitulino
Date Sept. 9, 2011, 8:25 p.m.
Message ID <1315599946-27081-5-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/114134/
State New
Headers show

Comments

Luiz Capitulino - Sept. 9, 2011, 8:25 p.m.
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.

This is a transition table as suggested by Lluís Vilanova.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 sysemu.h |    1 +
 vl.c     |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletions(-)
Luiz Capitulino - Sept. 14, 2011, 3:06 a.m.
On Fri,  9 Sep 2011 17:25:41 -0300
Luiz Capitulino <lcapitulino@redhat.com> 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.
> 
> This is a transition table as suggested by Lluís Vilanova.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

Would be nice to get a reviewed-by for this patch, as it wasn't trivial
to get it right and tested...

> ---
>  sysemu.h |    1 +
>  vl.c     |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 74 insertions(+), 1 deletions(-)
> 
> diff --git a/sysemu.h b/sysemu.h
> index 19088aa..a01ddac 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -36,6 +36,7 @@ extern uint8_t qemu_uuid[];
>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
>  
> +void runstate_init(void);
>  bool runstate_check(RunState state);
>  void runstate_set(RunState new_state);
>  typedef struct vm_change_state_entry VMChangeStateEntry;
> diff --git a/vl.c b/vl.c
> index 9926d2a..4a8edc7 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -327,14 +327,84 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>  
>  static RunState current_run_state = RSTATE_NO_STATE;
>  
> +typedef struct {
> +    RunState from;
> +    RunState to;
> +} RunStateTransition;
> +
> +static const RunStateTransition runstate_transitions_def[] = {
> +    /*     from      ->     to      */
> +    { RSTATE_NO_STATE, RSTATE_RUNNING },
> +    { RSTATE_NO_STATE, RSTATE_IN_MIGRATE },
> +    { RSTATE_NO_STATE, RSTATE_PRE_LAUNCH },
> +
> +    { RSTATE_DEBUG, RSTATE_RUNNING },
> +
> +    { RSTATE_IN_MIGRATE, RSTATE_RUNNING },
> +    { RSTATE_IN_MIGRATE, RSTATE_PRE_LAUNCH },
> +
> +    { RSTATE_PANICKED, RSTATE_PAUSED },
> +
> +    { RSTATE_IO_ERROR, RSTATE_RUNNING },
> +
> +    { RSTATE_PAUSED, RSTATE_RUNNING },
> +
> +    { RSTATE_POST_MIGRATE, RSTATE_RUNNING },
> +
> +    { RSTATE_PRE_LAUNCH, RSTATE_RUNNING },
> +    { RSTATE_PRE_LAUNCH, RSTATE_POST_MIGRATE },
> +
> +    { RSTATE_PRE_MIGRATE, RSTATE_RUNNING },
> +    { RSTATE_PRE_MIGRATE, RSTATE_POST_MIGRATE },
> +
> +    { RSTATE_RESTORE, RSTATE_RUNNING },
> +
> +    { RSTATE_RUNNING, RSTATE_DEBUG },
> +    { RSTATE_RUNNING, RSTATE_PANICKED },
> +    { RSTATE_RUNNING, RSTATE_IO_ERROR },
> +    { RSTATE_RUNNING, RSTATE_PAUSED },
> +    { RSTATE_RUNNING, RSTATE_PRE_MIGRATE },
> +    { RSTATE_RUNNING, RSTATE_RESTORE },
> +    { RSTATE_RUNNING, RSTATE_SAVEVM },
> +    { RSTATE_RUNNING, RSTATE_SHUTDOWN },
> +    { RSTATE_RUNNING, RSTATE_WATCHDOG },
> +
> +    { RSTATE_SAVEVM, RSTATE_RUNNING },
> +
> +    { RSTATE_SHUTDOWN, RSTATE_PAUSED },
> +
> +    { RSTATE_WATCHDOG, RSTATE_RUNNING },
> +
> +    { RSTATE_MAX, RSTATE_MAX },
> +};
> +
> +static bool runstate_valid_transitions[RSTATE_MAX][RSTATE_MAX];
> +
>  bool runstate_check(RunState state)
>  {
>      return current_run_state == state;
>  }
>  
> +void runstate_init(void)
> +{
> +    const RunStateTransition *p;
> +
> +    memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
> +
> +    for (p = &runstate_transitions_def[0]; p->from != RSTATE_MAX; p++) {
> +        runstate_valid_transitions[p->from][p->to] = true;
> +    }
> +}
> +
> +/* This function will abort() on invalid state transitions */
>  void runstate_set(RunState new_state)
>  {
> -    assert(new_state < RSTATE_MAX);
> +    if (new_state >= RSTATE_MAX ||
> +        !runstate_valid_transitions[current_run_state][new_state]) {
> +        fprintf(stderr, "invalid runstate transition\n");
> +        abort();
> +    }
> +
>      current_run_state = new_state;
>  }
>  
> @@ -2218,6 +2288,8 @@ int main(int argc, char **argv, char **envp)
>  
>      g_mem_set_vtable(&mem_trace);
>  
> +    runstate_init();
> +
>      init_clocks();
>  
>      qemu_cache_utils_init(envp);
Blue Swirl - Sept. 14, 2011, 7:55 p.m.
On Wed, Sep 14, 2011 at 3:06 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Fri,  9 Sep 2011 17:25:41 -0300
> Luiz Capitulino <lcapitulino@redhat.com> 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.
>>
>> This is a transition table as suggested by Lluís Vilanova.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>
> Would be nice to get a reviewed-by for this patch, as it wasn't trivial
> to get it right and tested...
>
>> ---
>>  sysemu.h |    1 +
>>  vl.c     |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 74 insertions(+), 1 deletions(-)
>>
>> diff --git a/sysemu.h b/sysemu.h
>> index 19088aa..a01ddac 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -36,6 +36,7 @@ extern uint8_t qemu_uuid[];
>>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
>>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
>>
>> +void runstate_init(void);
>>  bool runstate_check(RunState state);
>>  void runstate_set(RunState new_state);
>>  typedef struct vm_change_state_entry VMChangeStateEntry;
>> diff --git a/vl.c b/vl.c
>> index 9926d2a..4a8edc7 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -327,14 +327,84 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>>
>>  static RunState current_run_state = RSTATE_NO_STATE;
>>
>> +typedef struct {
>> +    RunState from;
>> +    RunState to;
>> +} RunStateTransition;
>> +
>> +static const RunStateTransition runstate_transitions_def[] = {
>> +    /*     from      ->     to      */
>> +    { RSTATE_NO_STATE, RSTATE_RUNNING },
>> +    { RSTATE_NO_STATE, RSTATE_IN_MIGRATE },
>> +    { RSTATE_NO_STATE, RSTATE_PRE_LAUNCH },
>> +
>> +    { RSTATE_DEBUG, RSTATE_RUNNING },
>> +
>> +    { RSTATE_IN_MIGRATE, RSTATE_RUNNING },
>> +    { RSTATE_IN_MIGRATE, RSTATE_PRE_LAUNCH },
>> +
>> +    { RSTATE_PANICKED, RSTATE_PAUSED },
>> +
>> +    { RSTATE_IO_ERROR, RSTATE_RUNNING },
>> +
>> +    { RSTATE_PAUSED, RSTATE_RUNNING },
>> +
>> +    { RSTATE_POST_MIGRATE, RSTATE_RUNNING },
>> +
>> +    { RSTATE_PRE_LAUNCH, RSTATE_RUNNING },
>> +    { RSTATE_PRE_LAUNCH, RSTATE_POST_MIGRATE },
>> +
>> +    { RSTATE_PRE_MIGRATE, RSTATE_RUNNING },
>> +    { RSTATE_PRE_MIGRATE, RSTATE_POST_MIGRATE },
>> +
>> +    { RSTATE_RESTORE, RSTATE_RUNNING },
>> +
>> +    { RSTATE_RUNNING, RSTATE_DEBUG },
>> +    { RSTATE_RUNNING, RSTATE_PANICKED },
>> +    { RSTATE_RUNNING, RSTATE_IO_ERROR },
>> +    { RSTATE_RUNNING, RSTATE_PAUSED },
>> +    { RSTATE_RUNNING, RSTATE_PRE_MIGRATE },
>> +    { RSTATE_RUNNING, RSTATE_RESTORE },
>> +    { RSTATE_RUNNING, RSTATE_SAVEVM },
>> +    { RSTATE_RUNNING, RSTATE_SHUTDOWN },
>> +    { RSTATE_RUNNING, RSTATE_WATCHDOG },
>> +
>> +    { RSTATE_SAVEVM, RSTATE_RUNNING },
>> +
>> +    { RSTATE_SHUTDOWN, RSTATE_PAUSED },
>> +
>> +    { RSTATE_WATCHDOG, RSTATE_RUNNING },
>> +
>> +    { RSTATE_MAX, RSTATE_MAX },

I don't see a state which requires a system_reset to recover, which I
think was the original motivation. Do I miss something?

Otherwise the patch looks fine.

>> +};
>> +
>> +static bool runstate_valid_transitions[RSTATE_MAX][RSTATE_MAX];
>> +
>>  bool runstate_check(RunState state)
>>  {
>>      return current_run_state == state;
>>  }
>>
>> +void runstate_init(void)
>> +{
>> +    const RunStateTransition *p;
>> +
>> +    memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
>> +
>> +    for (p = &runstate_transitions_def[0]; p->from != RSTATE_MAX; p++) {
>> +        runstate_valid_transitions[p->from][p->to] = true;
>> +    }
>> +}
>> +
>> +/* This function will abort() on invalid state transitions */
>>  void runstate_set(RunState new_state)
>>  {
>> -    assert(new_state < RSTATE_MAX);
>> +    if (new_state >= RSTATE_MAX ||
>> +        !runstate_valid_transitions[current_run_state][new_state]) {
>> +        fprintf(stderr, "invalid runstate transition\n");
>> +        abort();
>> +    }
>> +
>>      current_run_state = new_state;
>>  }
>>
>> @@ -2218,6 +2288,8 @@ int main(int argc, char **argv, char **envp)
>>
>>      g_mem_set_vtable(&mem_trace);
>>
>> +    runstate_init();
>> +
>>      init_clocks();
>>
>>      qemu_cache_utils_init(envp);
>
>
>
Luiz Capitulino - Sept. 14, 2011, 8:23 p.m.
On Wed, 14 Sep 2011 19:55:25 +0000
Blue Swirl <blauwirbel@gmail.com> wrote:

> On Wed, Sep 14, 2011 at 3:06 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Fri,  9 Sep 2011 17:25:41 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> 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.
> >>
> >> This is a transition table as suggested by Lluís Vilanova.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >
> > Would be nice to get a reviewed-by for this patch, as it wasn't trivial
> > to get it right and tested...
> >
> >> ---
> >>  sysemu.h |    1 +
> >>  vl.c     |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 74 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/sysemu.h b/sysemu.h
> >> index 19088aa..a01ddac 100644
> >> --- a/sysemu.h
> >> +++ b/sysemu.h
> >> @@ -36,6 +36,7 @@ extern uint8_t qemu_uuid[];
> >>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
> >>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> >>
> >> +void runstate_init(void);
> >>  bool runstate_check(RunState state);
> >>  void runstate_set(RunState new_state);
> >>  typedef struct vm_change_state_entry VMChangeStateEntry;
> >> diff --git a/vl.c b/vl.c
> >> index 9926d2a..4a8edc7 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -327,14 +327,84 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> >>
> >>  static RunState current_run_state = RSTATE_NO_STATE;
> >>
> >> +typedef struct {
> >> +    RunState from;
> >> +    RunState to;
> >> +} RunStateTransition;
> >> +
> >> +static const RunStateTransition runstate_transitions_def[] = {
> >> +    /*     from      ->     to      */
> >> +    { RSTATE_NO_STATE, RSTATE_RUNNING },
> >> +    { RSTATE_NO_STATE, RSTATE_IN_MIGRATE },
> >> +    { RSTATE_NO_STATE, RSTATE_PRE_LAUNCH },
> >> +
> >> +    { RSTATE_DEBUG, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_IN_MIGRATE, RSTATE_RUNNING },
> >> +    { RSTATE_IN_MIGRATE, RSTATE_PRE_LAUNCH },
> >> +
> >> +    { RSTATE_PANICKED, RSTATE_PAUSED },
> >> +
> >> +    { RSTATE_IO_ERROR, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_PAUSED, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_POST_MIGRATE, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_PRE_LAUNCH, RSTATE_RUNNING },
> >> +    { RSTATE_PRE_LAUNCH, RSTATE_POST_MIGRATE },
> >> +
> >> +    { RSTATE_PRE_MIGRATE, RSTATE_RUNNING },
> >> +    { RSTATE_PRE_MIGRATE, RSTATE_POST_MIGRATE },
> >> +
> >> +    { RSTATE_RESTORE, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_RUNNING, RSTATE_DEBUG },
> >> +    { RSTATE_RUNNING, RSTATE_PANICKED },
> >> +    { RSTATE_RUNNING, RSTATE_IO_ERROR },
> >> +    { RSTATE_RUNNING, RSTATE_PAUSED },
> >> +    { RSTATE_RUNNING, RSTATE_PRE_MIGRATE },
> >> +    { RSTATE_RUNNING, RSTATE_RESTORE },
> >> +    { RSTATE_RUNNING, RSTATE_SAVEVM },
> >> +    { RSTATE_RUNNING, RSTATE_SHUTDOWN },
> >> +    { RSTATE_RUNNING, RSTATE_WATCHDOG },
> >> +
> >> +    { RSTATE_SAVEVM, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_SHUTDOWN, RSTATE_PAUSED },
> >> +
> >> +    { RSTATE_WATCHDOG, RSTATE_RUNNING },
> >> +
> >> +    { RSTATE_MAX, RSTATE_MAX },
> 
> I don't see a state which requires a system_reset to recover, which I
> think was the original motivation. Do I miss something?

That's done by patch 7/9 Monitor/QMP: Don't allow cont on bad VM state.

> Otherwise the patch looks fine.

Thanks!

> 
> >> +};
> >> +
> >> +static bool runstate_valid_transitions[RSTATE_MAX][RSTATE_MAX];
> >> +
> >>  bool runstate_check(RunState state)
> >>  {
> >>      return current_run_state == state;
> >>  }
> >>
> >> +void runstate_init(void)
> >> +{
> >> +    const RunStateTransition *p;
> >> +
> >> +    memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
> >> +
> >> +    for (p = &runstate_transitions_def[0]; p->from != RSTATE_MAX; p++) {
> >> +        runstate_valid_transitions[p->from][p->to] = true;
> >> +    }
> >> +}
> >> +
> >> +/* This function will abort() on invalid state transitions */
> >>  void runstate_set(RunState new_state)
> >>  {
> >> -    assert(new_state < RSTATE_MAX);
> >> +    if (new_state >= RSTATE_MAX ||
> >> +        !runstate_valid_transitions[current_run_state][new_state]) {
> >> +        fprintf(stderr, "invalid runstate transition\n");
> >> +        abort();
> >> +    }
> >> +
> >>      current_run_state = new_state;
> >>  }
> >>
> >> @@ -2218,6 +2288,8 @@ int main(int argc, char **argv, char **envp)
> >>
> >>      g_mem_set_vtable(&mem_trace);
> >>
> >> +    runstate_init();
> >> +
> >>      init_clocks();
> >>
> >>      qemu_cache_utils_init(envp);
> >
> >
> >
>

Patch

diff --git a/sysemu.h b/sysemu.h
index 19088aa..a01ddac 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -36,6 +36,7 @@  extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
+void runstate_init(void);
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 typedef struct vm_change_state_entry VMChangeStateEntry;
diff --git a/vl.c b/vl.c
index 9926d2a..4a8edc7 100644
--- a/vl.c
+++ b/vl.c
@@ -327,14 +327,84 @@  static int default_driver_check(QemuOpts *opts, void *opaque)
 
 static RunState current_run_state = RSTATE_NO_STATE;
 
+typedef struct {
+    RunState from;
+    RunState to;
+} RunStateTransition;
+
+static const RunStateTransition runstate_transitions_def[] = {
+    /*     from      ->     to      */
+    { RSTATE_NO_STATE, RSTATE_RUNNING },
+    { RSTATE_NO_STATE, RSTATE_IN_MIGRATE },
+    { RSTATE_NO_STATE, RSTATE_PRE_LAUNCH },
+
+    { RSTATE_DEBUG, RSTATE_RUNNING },
+
+    { RSTATE_IN_MIGRATE, RSTATE_RUNNING },
+    { RSTATE_IN_MIGRATE, RSTATE_PRE_LAUNCH },
+
+    { RSTATE_PANICKED, RSTATE_PAUSED },
+
+    { RSTATE_IO_ERROR, RSTATE_RUNNING },
+
+    { RSTATE_PAUSED, RSTATE_RUNNING },
+
+    { RSTATE_POST_MIGRATE, RSTATE_RUNNING },
+
+    { RSTATE_PRE_LAUNCH, RSTATE_RUNNING },
+    { RSTATE_PRE_LAUNCH, RSTATE_POST_MIGRATE },
+
+    { RSTATE_PRE_MIGRATE, RSTATE_RUNNING },
+    { RSTATE_PRE_MIGRATE, RSTATE_POST_MIGRATE },
+
+    { RSTATE_RESTORE, RSTATE_RUNNING },
+
+    { RSTATE_RUNNING, RSTATE_DEBUG },
+    { RSTATE_RUNNING, RSTATE_PANICKED },
+    { RSTATE_RUNNING, RSTATE_IO_ERROR },
+    { RSTATE_RUNNING, RSTATE_PAUSED },
+    { RSTATE_RUNNING, RSTATE_PRE_MIGRATE },
+    { RSTATE_RUNNING, RSTATE_RESTORE },
+    { RSTATE_RUNNING, RSTATE_SAVEVM },
+    { RSTATE_RUNNING, RSTATE_SHUTDOWN },
+    { RSTATE_RUNNING, RSTATE_WATCHDOG },
+
+    { RSTATE_SAVEVM, RSTATE_RUNNING },
+
+    { RSTATE_SHUTDOWN, RSTATE_PAUSED },
+
+    { RSTATE_WATCHDOG, RSTATE_RUNNING },
+
+    { RSTATE_MAX, RSTATE_MAX },
+};
+
+static bool runstate_valid_transitions[RSTATE_MAX][RSTATE_MAX];
+
 bool runstate_check(RunState state)
 {
     return current_run_state == state;
 }
 
+void runstate_init(void)
+{
+    const RunStateTransition *p;
+
+    memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
+
+    for (p = &runstate_transitions_def[0]; p->from != RSTATE_MAX; p++) {
+        runstate_valid_transitions[p->from][p->to] = true;
+    }
+}
+
+/* This function will abort() on invalid state transitions */
 void runstate_set(RunState new_state)
 {
-    assert(new_state < RSTATE_MAX);
+    if (new_state >= RSTATE_MAX ||
+        !runstate_valid_transitions[current_run_state][new_state]) {
+        fprintf(stderr, "invalid runstate transition\n");
+        abort();
+    }
+
     current_run_state = new_state;
 }
 
@@ -2218,6 +2288,8 @@  int main(int argc, char **argv, char **envp)
 
     g_mem_set_vtable(&mem_trace);
 
+    runstate_init();
+
     init_clocks();
 
     qemu_cache_utils_init(envp);