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 },
Anyway I agree with your patch. How about we drop RUN_STATE_DEBUG and drop RUN_STATE_GUEST_PANICKED from need reset list?
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