Re: [libvirt] pvpanic plans?
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(a)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