Re: [Qemu-devel] pvpanic plans?

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 31, 2013 at 05:38:40PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 17:26, Michael S. Tsirkin ha scritto:
> > On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote:
> >> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto:
> >>>> PANICKED->DEBUG was added by commit bc7d0e667.  That commit can be
> >>>> reverted if the panicked state is removed from runstate_needs_reset.
> >>>
> >>> Okay so let's drop the code duplication and explicitly make
> >>> them the same?
> >>>
> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>
> >>>
> >>> diff --git a/vl.c b/vl.c
> >>> index 46c29c4..e12d317 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -638,10 +638,6 @@ static const RunStateTransition runstate_transitions_def[] = {
> >>>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
> >>>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
> >>>  
> >>> -    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> >>> -    { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> >>> -    { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> >>> -
> >>>      { RUN_STATE_MAX, RUN_STATE_MAX },
> >>>  };
> >>>  
> >>> @@ -660,6 +656,12 @@ static void runstate_init(void)
> >>>  
> >>>      for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
> >>>          runstate_valid_transitions[p->from][p->to] = true;
> >>> +        /* Panicked state is same as paused, we only made it different so
> >>> +         * management can detect a panic.
> >>> +         */
> >>> +        if (p->from == RUN_STATE_PAUSED) {
> >>> +            runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = true;
> >>
> >> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as
> >> well, and perhaps there are others I'm missing.  Just add a comment
> >> before runstate_transitions_def's entries for PANICKED, IO_ERROR and
> >> WATCHDOG.
> >>
> >> But again, it is somewhat separate from the issue at hand, which is to
> >> finally make pvpanic usable and hopefully before 1.7.
> >>
> >> Paolo
> > 
> > The issue is that you can't continue from panicked state.
> > You should be able to do that without going through paused.
> 
> Yes, that's what my patch (posted the link before) does:
> 
> -    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING },
>      { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
> -    { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
> 
> 
> Comments don't compile, but are also easier to understand than code.
> Special logic in runstate_init is unnecessarily complicated, for a table
> that hardly sees any change.  English works better, whoever modifies the
> table has it under their eyes.
> 
> Paolo

But code duplication is bad. I think IO error for example
is broken in that you can't pause but can run then pause.
Seems strange.
Internal error has same bug as panicked.

So it's the same bug for a bunch of states, let's just
have a way to say "this is same as paused".
How's this?

diff --git a/vl.c b/vl.c
index 46c29c4..4388c95 100644
--- a/vl.c
+++ b/vl.c
@@ -593,12 +593,6 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
 
-    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
-    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
-
-    { RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
-    { RUN_STATE_IO_ERROR, RUN_STATE_FINISH_MIGRATE },
-
     { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
     { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
 
@@ -635,16 +629,17 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
     { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
 
-    { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
-    { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
-
-    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
-    { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE },
-    { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG },
-
     { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
+static const RunState runstate_paused[] = {
+    { RUN_STATE_GUEST_PANICKED},
+    { RUN_STATE_IO_ERROR},
+    { RUN_STATE_INTERNAL_ERROR},
+    { RUN_STATE_WATCHDOG},
+    { RUN_STATE_MAX },
+};
+
 static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX];
 
 bool runstate_check(RunState state)
@@ -655,12 +650,21 @@ bool runstate_check(RunState state)
 static void runstate_init(void)
 {
     const RunStateTransition *p;
+    const RunState *i;
 
     memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
 
     for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) {
         runstate_valid_transitions[p->from][p->to] = true;
     }
+    /* Allow two-way transitions between identical states */
+    for (i = &runstate_paused[0]; *p != RUN_STATE_MAX; p++) {
+        runstate_valid_transitions[*i][RUN_STATE_PAUSED] = true;
+        runstate_valid_transitions[RUN_STATE_PAUSED][*i] = true;
+        memcpy(&runstate_valid_transitions[*i],
+               &runstate_valid_transitions[RUN_STATE_PAUSED],
+                sizeof(runstate_valid_transitions[RUN_STATE_PAUSED]));
+    }
 }
 
 /* This function will abort() on invalid state transitions */
@@ -686,8 +690,6 @@ int runstate_is_running(void)
 bool runstate_needs_reset(void)
 {
-    return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-        runstate_check(RUN_STATE_SHUTDOWN) ||
-        runstate_check(RUN_STATE_GUEST_PANICKED);
+    return runstate_check(RUN_STATE_SHUTDOWN);
 }
 
 StatusInfo *qmp_query_status(Error **errp)