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@xxxxxxxxxx>
> >>>
> >>>
> >>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list