Patchwork [6/7] Monitor: Don't allow cont on bad VM state

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 3, 2011, 3:17 p.m.
Message ID <1312384643-581-7-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/108273/
State New
Headers show

Comments

Luiz Capitulino - Aug. 3, 2011, 3:17 p.m.
We have two states where issuing cont before system_reset can be
catastrophic: QSTATE_SHUTDOWN (when -no-shutdown is used) and
QSTATE_INTERROR (which only happen with kvm).

This commit fixes that by making system_reset mandatory before
issuing cont in those states.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cpus.c    |    4 ++++
 monitor.c |    8 ++++++++
 qerror.c  |    4 ++++
 qerror.h  |    3 +++
 sysemu.h  |    2 +-
 vl.c      |    1 +
 6 files changed, 21 insertions(+), 1 deletions(-)
Jan Kiszka - Aug. 3, 2011, 3:32 p.m.
On 2011-08-03 17:17, Luiz Capitulino wrote:
> We have two states where issuing cont before system_reset can be
> catastrophic: QSTATE_SHUTDOWN (when -no-shutdown is used) and
> QSTATE_INTERROR (which only happen with kvm).
> 
> This commit fixes that by making system_reset mandatory before
> issuing cont in those states.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  cpus.c    |    4 ++++
>  monitor.c |    8 ++++++++
>  qerror.c  |    4 ++++
>  qerror.h  |    3 +++
>  sysemu.h  |    2 +-
>  vl.c      |    1 +
>  6 files changed, 21 insertions(+), 1 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 65ea503..a61e658 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -125,6 +125,10 @@ static void do_vm_stop(QemuState state)
>          pause_all_vcpus();
>          qemu_state_set(state);
>          vm_state_notify(0, state);
> +        if (state == QSTATE_INTERROR || state == QSTATE_SHUTDOWN) {
> +            /* system_reset is required by 'cont' */
> +            system_reset_required = 1;
> +        }
>          qemu_aio_flush();
>          bdrv_flush_all();
>          monitor_protocol_event(QEVENT_STOP, NULL);
> diff --git a/monitor.c b/monitor.c
> index 3fa2cf7..f1cb5af 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1312,7 +1312,14 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      if (qemu_state_get() == QSTATE_INMIGRATE) {
>          qerror_report(QERR_MIGRATION_EXPECTED);
>          return -1;
> +    } else if (qemu_state_get() == QSTATE_INTERROR ||
> +               qemu_state_get() == QSTATE_SHUTDOWN) {
> +        if (system_reset_required) {
> +            qerror_report(QERR_RESET_REQUIRED);
> +            return -1;
> +        }

Why not just enter a proper state, likely QSTATE_PAUSED, when resetting
over INTERROR or SHUTDOWN? Would save you system_reset_required and make
the state machine simpler.

Jan
Luiz Capitulino - Aug. 3, 2011, 5:32 p.m.
On Wed, 03 Aug 2011 17:32:03 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2011-08-03 17:17, Luiz Capitulino wrote:
> > We have two states where issuing cont before system_reset can be
> > catastrophic: QSTATE_SHUTDOWN (when -no-shutdown is used) and
> > QSTATE_INTERROR (which only happen with kvm).
> > 
> > This commit fixes that by making system_reset mandatory before
> > issuing cont in those states.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  cpus.c    |    4 ++++
> >  monitor.c |    8 ++++++++
> >  qerror.c  |    4 ++++
> >  qerror.h  |    3 +++
> >  sysemu.h  |    2 +-
> >  vl.c      |    1 +
> >  6 files changed, 21 insertions(+), 1 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 65ea503..a61e658 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -125,6 +125,10 @@ static void do_vm_stop(QemuState state)
> >          pause_all_vcpus();
> >          qemu_state_set(state);
> >          vm_state_notify(0, state);
> > +        if (state == QSTATE_INTERROR || state == QSTATE_SHUTDOWN) {
> > +            /* system_reset is required by 'cont' */
> > +            system_reset_required = 1;
> > +        }
> >          qemu_aio_flush();
> >          bdrv_flush_all();
> >          monitor_protocol_event(QEVENT_STOP, NULL);
> > diff --git a/monitor.c b/monitor.c
> > index 3fa2cf7..f1cb5af 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1312,7 +1312,14 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >      if (qemu_state_get() == QSTATE_INMIGRATE) {
> >          qerror_report(QERR_MIGRATION_EXPECTED);
> >          return -1;
> > +    } else if (qemu_state_get() == QSTATE_INTERROR ||
> > +               qemu_state_get() == QSTATE_SHUTDOWN) {
> > +        if (system_reset_required) {
> > +            qerror_report(QERR_RESET_REQUIRED);
> > +            return -1;
> > +        }
> 
> Why not just enter a proper state, likely QSTATE_PAUSED, when resetting
> over INTERROR or SHUTDOWN? Would save you system_reset_required and make
> the state machine simpler.

Yes, seems to be a good idea.
Jan Kiszka - Aug. 4, 2011, 8:42 a.m.
On 2011-08-03 19:32, Luiz Capitulino wrote:
> On Wed, 03 Aug 2011 17:32:03 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2011-08-03 17:17, Luiz Capitulino wrote:
>>> We have two states where issuing cont before system_reset can be
>>> catastrophic: QSTATE_SHUTDOWN (when -no-shutdown is used) and
>>> QSTATE_INTERROR (which only happen with kvm).
>>>
>>> This commit fixes that by making system_reset mandatory before
>>> issuing cont in those states.
>>>
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>  cpus.c    |    4 ++++
>>>  monitor.c |    8 ++++++++
>>>  qerror.c  |    4 ++++
>>>  qerror.h  |    3 +++
>>>  sysemu.h  |    2 +-
>>>  vl.c      |    1 +
>>>  6 files changed, 21 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 65ea503..a61e658 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -125,6 +125,10 @@ static void do_vm_stop(QemuState state)
>>>          pause_all_vcpus();
>>>          qemu_state_set(state);
>>>          vm_state_notify(0, state);
>>> +        if (state == QSTATE_INTERROR || state == QSTATE_SHUTDOWN) {
>>> +            /* system_reset is required by 'cont' */
>>> +            system_reset_required = 1;
>>> +        }
>>>          qemu_aio_flush();
>>>          bdrv_flush_all();
>>>          monitor_protocol_event(QEVENT_STOP, NULL);
>>> diff --git a/monitor.c b/monitor.c
>>> index 3fa2cf7..f1cb5af 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1312,7 +1312,14 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>>      if (qemu_state_get() == QSTATE_INMIGRATE) {
>>>          qerror_report(QERR_MIGRATION_EXPECTED);
>>>          return -1;
>>> +    } else if (qemu_state_get() == QSTATE_INTERROR ||
>>> +               qemu_state_get() == QSTATE_SHUTDOWN) {
>>> +        if (system_reset_required) {
>>> +            qerror_report(QERR_RESET_REQUIRED);
>>> +            return -1;
>>> +        }
>>
>> Why not just enter a proper state, likely QSTATE_PAUSED, when resetting
>> over INTERROR or SHUTDOWN? Would save you system_reset_required and make
>> the state machine simpler.
> 
> Yes, seems to be a good idea.

Forgot to say that the rest looks OK to me.

Jan

Patch

diff --git a/cpus.c b/cpus.c
index 65ea503..a61e658 100644
--- a/cpus.c
+++ b/cpus.c
@@ -125,6 +125,10 @@  static void do_vm_stop(QemuState state)
         pause_all_vcpus();
         qemu_state_set(state);
         vm_state_notify(0, state);
+        if (state == QSTATE_INTERROR || state == QSTATE_SHUTDOWN) {
+            /* system_reset is required by 'cont' */
+            system_reset_required = 1;
+        }
         qemu_aio_flush();
         bdrv_flush_all();
         monitor_protocol_event(QEVENT_STOP, NULL);
diff --git a/monitor.c b/monitor.c
index 3fa2cf7..f1cb5af 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1312,7 +1312,14 @@  static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (qemu_state_get() == QSTATE_INMIGRATE) {
         qerror_report(QERR_MIGRATION_EXPECTED);
         return -1;
+    } else if (qemu_state_get() == QSTATE_INTERROR ||
+               qemu_state_get() == QSTATE_SHUTDOWN) {
+        if (system_reset_required) {
+            qerror_report(QERR_RESET_REQUIRED);
+            return -1;
+        }
     }
+
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err) {
@@ -2014,6 +2021,7 @@  static int do_system_reset(Monitor *mon, const QDict *qdict,
                            QObject **ret_data)
 {
     qemu_system_reset_request();
+    system_reset_required = 0;
     return 0;
 }
 
diff --git a/qerror.c b/qerror.c
index 69c1bc9..0dd65a1 100644
--- a/qerror.c
+++ b/qerror.c
@@ -194,6 +194,10 @@  static const QErrorStringTable qerror_table[] = {
         .desc      = "QMP input object member '%(member)' is unexpected",
     },
     {
+        .error_fmt = QERR_RESET_REQUIRED,
+        .desc      = "Resetting the Virtual Machine is required",
+    },
+    {
         .error_fmt = QERR_SET_PASSWD_FAILED,
         .desc      = "Could not set password",
     },
diff --git a/qerror.h b/qerror.h
index 8058456..d407001 100644
--- a/qerror.h
+++ b/qerror.h
@@ -163,6 +163,9 @@  QError *qobject_to_qerror(const QObject *obj);
 #define QERR_QMP_EXTRA_MEMBER \
     "{ 'class': 'QMPExtraInputObjectMember', 'data': { 'member': %s } }"
 
+#define QERR_RESET_REQUIRED \
+    "{ 'class': 'ResetRequired', 'data': {} }"
+
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
diff --git a/sysemu.h b/sysemu.h
index 46079ab..12a3f6a 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -30,7 +30,7 @@  typedef enum {
 } QemuState;
 
 extern const char *bios_name;
-
+extern int system_reset_required;
 extern const char *qemu_name;
 extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
diff --git a/vl.c b/vl.c
index 65cf4a5..7fad355 100644
--- a/vl.c
+++ b/vl.c
@@ -183,6 +183,7 @@  int mem_prealloc = 0; /* force preallocation of physical target memory */
 #endif
 int nb_nics;
 NICInfo nd_table[MAX_NICS];
+int system_reset_required = 0;
 int autostart;
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */