[Qemu-devel] [PATCH 0/7]: Introduce the QemuState type

[Qemu-devel] [PATCH 0/7]: Introduce the QemuState type

[Luiz Capitulino]

It replaces the VMSTOP macros and allows us to drop some global variables.

Also, the problem with issuing 'cont' when the VM is in bad state is addressed,
and we make the current state available in QMP.

 audio/audio.c      |    2 +-
 cpus.c             |   26 +++++++++++--------
 cpus.h             |    1 -
 gdbstub.c          |   34 +++++++++++++-------------
 hw/etraxfs_dma.c   |    2 +-
 hw/ide/ahci.c      |    2 +-
 hw/ide/core.c      |    4 +-
 hw/ide/internal.h  |    3 +-
 hw/ide/pci.c       |    2 +-
 hw/kvmclock.c      |    5 ++-
 hw/qxl.c           |    3 +-
 hw/scsi-disk.c     |    4 +-
 hw/virtio-blk.c    |    5 ++-
 hw/virtio.c        |    4 +-
 hw/watchdog.c      |    2 +-
 kvm-all.c          |    2 +-
 migration.c        |   14 +++++++----
 monitor.c          |   19 ++++++++++----
 qemu-timer.c       |   11 ++++----
 qerror.c           |    4 +++
 qerror.h           |    3 ++
 qmp-commands.hx    |   21 +++++++++++++++-
 savevm.c           |    8 +++---
 sysemu.h           |   42 +++++++++++++++++++++----------
 target-i386/kvm.c  |    4 +-
 ui/sdl.c           |    6 ++--
 ui/spice-display.c |    3 +-
 vl.c               |   68 +++++++++++++++++++++++++++++++++++++++++++--------
 xen-all.c          |    8 ++++--
 29 files changed, 210 insertions(+), 102 deletions(-)

[Qemu-devel] [PATCH 1/7] Move vm_state_notify() prototype from cpus.h to sysemu.h

[Luiz Capitulino]

It's where all state handling functions prototypes are located.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cpus.h   |    1 -
 sysemu.h |    1 +
 2 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cpus.h b/cpus.h
index f42b54e..5885885 100644
--- a/cpus.h
+++ b/cpus.h
@@ -15,7 +15,6 @@ void cpu_synchronize_all_post_init(void);
 /* vl.c */
 extern int smp_cores;
 extern int smp_threads;
-void vm_state_notify(int running, int reason);
 bool cpu_exec_all(void);
 void set_numa_modes(void);
 void set_cpu_log(const char *optarg);
diff --git a/sysemu.h b/sysemu.h
index bd830e5..61ad4fb 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -23,6 +23,7 @@ typedef void VMChangeStateHandler(void *opaque, int running, int reason);
 VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
                                                      void *opaque);
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
+void vm_state_notify(int running, int reason);
 
 #define VMSTOP_USER      0
 #define VMSTOP_DEBUG     1
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

Today, when notifying a VM state change with vm_state_notify(), we
pass a VMSTOP macro as the 'reason' argument. This is not ideal,
because the VMSTOP macros tells why qemu stopped and not exactly
what the current VM state is.

One example to demonstrate this problem is that vm_start() calls
vm_state_notify() with reason=0, which turns out to be VMSTOP_USER.

This commit fixes that by replacing the VMSTOP macros with
a proper QemuState type.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 audio/audio.c      |    2 +-
 cpus.c             |   12 ++++++------
 gdbstub.c          |   30 +++++++++++++++---------------
 hw/ide/ahci.c      |    2 +-
 hw/ide/core.c      |    4 ++--
 hw/ide/internal.h  |    3 ++-
 hw/ide/pci.c       |    2 +-
 hw/kvmclock.c      |    3 ++-
 hw/qxl.c           |    3 ++-
 hw/scsi-disk.c     |    4 ++--
 hw/virtio-blk.c    |    5 +++--
 hw/virtio.c        |    2 +-
 hw/watchdog.c      |    2 +-
 kvm-all.c          |    2 +-
 migration.c        |    2 +-
 monitor.c          |    4 ++--
 qemu-timer.c       |    3 ++-
 savevm.c           |    4 ++--
 sysemu.h           |   30 +++++++++++++++++-------------
 target-i386/kvm.c  |    2 +-
 ui/spice-display.c |    3 ++-
 vl.c               |   12 ++++++------
 xen-all.c          |    6 ++++--
 23 files changed, 77 insertions(+), 65 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 50d2b64..254cffc 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1743,7 +1743,7 @@ static int audio_driver_init (AudioState *s, struct audio_driver *drv)
 }
 
 static void audio_vm_change_state_handler (void *opaque, int running,
-                                           int reason)
+                                           QemuState state)
 {
     AudioState *s = opaque;
     HWVoiceOut *hwo = NULL;
diff --git a/cpus.c b/cpus.c
index 6bf4e3f..ebbb8b9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -118,13 +118,13 @@ int cpu_is_stopped(CPUState *env)
     return !vm_running || env->stopped;
 }
 
-static void do_vm_stop(int reason)
+static void do_vm_stop(QemuState state)
 {
     if (vm_running) {
         cpu_disable_ticks();
         vm_running = 0;
         pause_all_vcpus();
-        vm_state_notify(0, reason);
+        vm_state_notify(0, state);
         qemu_aio_flush();
         bdrv_flush_all();
         monitor_protocol_event(QEVENT_STOP, NULL);
@@ -628,9 +628,9 @@ void cpu_stop_current(void)
 {
 }
 
-void vm_stop(int reason)
+void vm_stop(QemuState state)
 {
-    do_vm_stop(reason);
+    do_vm_stop(state);
 }
 
 #else /* CONFIG_IOTHREAD */
@@ -1022,7 +1022,7 @@ void cpu_stop_current(void)
     }
 }
 
-void vm_stop(int reason)
+void vm_stop(QemuState state)
 {
     if (!qemu_thread_is_self(&io_thread)) {
         qemu_system_vmstop_request(reason);
@@ -1033,7 +1033,7 @@ void vm_stop(int reason)
         cpu_stop_current();
         return;
     }
-    do_vm_stop(reason);
+    do_vm_stop(state);
 }
 
 #endif
diff --git a/gdbstub.c b/gdbstub.c
index 27b0cfa..ae57d6a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2267,7 +2267,7 @@ void gdb_set_stop_cpu(CPUState *env)
 }
 
 #ifndef CONFIG_USER_ONLY
-static void gdb_vm_state_change(void *opaque, int running, int reason)
+static void gdb_vm_state_change(void *opaque, int running, QemuState state)
 {
     GDBState *s = gdbserver_state;
     CPUState *env = s->c_cpu;
@@ -2278,8 +2278,8 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
     if (running || s->state == RS_INACTIVE || s->state == RS_SYSCALL) {
         return;
     }
-    switch (reason) {
-    case VMSTOP_DEBUG:
+    switch (state) {
+    case QSTATE_DEBUG:
         if (env->watchpoint_hit) {
             switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
@@ -2302,25 +2302,25 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
         tb_flush(env);
         ret = GDB_SIGNAL_TRAP;
         break;
-    case VMSTOP_USER:
+    case QSTATE_PAUSED:
         ret = GDB_SIGNAL_INT;
         break;
-    case VMSTOP_SHUTDOWN:
+    case QSTATE_SHUTDOWN:
         ret = GDB_SIGNAL_QUIT;
         break;
-    case VMSTOP_DISKFULL:
+    case QSTATE_IOERROR:
         ret = GDB_SIGNAL_IO;
         break;
-    case VMSTOP_WATCHDOG:
+    case QSTATE_WATCHDOG:
         ret = GDB_SIGNAL_ALRM;
         break;
-    case VMSTOP_PANIC:
+    case QSTATE_INTERROR:
         ret = GDB_SIGNAL_ABRT;
         break;
-    case VMSTOP_SAVEVM:
-    case VMSTOP_LOADVM:
+    case QSTATE_SAVEVM:
+    case QSTATE_RESTVM:
         return;
-    case VMSTOP_MIGRATE:
+    case QSTATE_PREMIGRATE:
         ret = GDB_SIGNAL_XCPU;
         break;
     default:
@@ -2357,7 +2357,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
     gdb_current_syscall_cb = cb;
     s->state = RS_SYSCALL;
 #ifndef CONFIG_USER_ONLY
-    vm_stop(VMSTOP_DEBUG);
+    vm_stop(QSTATE_DEBUG);
 #endif
     s->state = RS_IDLE;
     va_start(va, fmt);
@@ -2431,7 +2431,7 @@ static void gdb_read_byte(GDBState *s, int ch)
     if (vm_running) {
         /* when the CPU is running, we cannot do anything except stop
            it when receiving a char */
-        vm_stop(VMSTOP_USER);
+        vm_stop(QSTATE_PAUSED);
     } else
 #endif
     {
@@ -2693,7 +2693,7 @@ static void gdb_chr_event(void *opaque, int event)
 {
     switch (event) {
     case CHR_EVENT_OPENED:
-        vm_stop(VMSTOP_USER);
+        vm_stop(QSTATE_PAUSED);
         gdb_has_xml = 0;
         break;
     default:
@@ -2734,7 +2734,7 @@ static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len)
 static void gdb_sigterm_handler(int signal)
 {
     if (vm_running) {
-        vm_stop(VMSTOP_USER);
+        vm_stop(QSTATE_PAUSED);
     }
 }
 #endif
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1f008a3..ec9a727 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1102,7 +1102,7 @@ static void ahci_irq_set(void *opaque, int n, int level)
 {
 }
 
-static void ahci_dma_restart_cb(void *opaque, int running, int reason)
+static void ahci_dma_restart_cb(void *opaque, int running, QemuState state)
 {
 }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d145b19..8c5a52b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -523,7 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->error_status = op;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(VMSTOP_DISKFULL);
+        vm_stop(QSTATE_IOERROR);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
             dma_buf_commit(s, 0);
@@ -1815,7 +1815,7 @@ static int ide_nop_int(IDEDMA *dma, int x)
     return 0;
 }
 
-static void ide_nop_restart(void *opaque, int x, int y)
+static void ide_nop_restart(void *opaque, int x, QemuState y)
 {
 }
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 02e805f..fb8e3e5 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -10,6 +10,7 @@
 #include "block_int.h"
 #include "iorange.h"
 #include "dma.h"
+#include "sysemu.h"
 
 /* debug IDE devices */
 //#define DEBUG_IDE
@@ -379,7 +380,7 @@ typedef void EndTransferFunc(IDEState *);
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
 typedef int DMAFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
-typedef void DMARestartFunc(void *, int, int);
+typedef void DMARestartFunc(void *, int, QemuState);
 
 struct unreported_events {
     bool eject_request;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 9f3050a..427829e 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -223,7 +223,7 @@ static void bmdma_restart_bh(void *opaque)
     }
 }
 
-static void bmdma_restart_cb(void *opaque, int running, int reason)
+static void bmdma_restart_cb(void *opaque, int running, QemuState state)
 {
     IDEDMA *dma = opaque;
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
diff --git a/hw/kvmclock.c b/hw/kvmclock.c
index 692ad18..de5a655 100644
--- a/hw/kvmclock.c
+++ b/hw/kvmclock.c
@@ -59,7 +59,8 @@ static int kvmclock_post_load(void *opaque, int version_id)
     return kvm_vm_ioctl(kvm_state, KVM_SET_CLOCK, &data);
 }
 
-static void kvmclock_vm_state_change(void *opaque, int running, int reason)
+static void kvmclock_vm_state_change(void *opaque, int running,
+                                     QemuState state)
 {
     KVMClockState *s = opaque;
 
diff --git a/hw/qxl.c b/hw/qxl.c
index a6fb7f0..98217a6 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1180,7 +1180,8 @@ static void qxl_hw_text_update(void *opaque, console_ch_t *chardata)
     }
 }
 
-static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
+static void qxl_vm_change_state_handler(void *opaque, int running,
+                                        QemuState state)
 {
     PCIQXLDevice *qxl = opaque;
     qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f42a5d1..643781c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -215,7 +215,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         r->status |= SCSI_REQ_STATUS_RETRY | type;
 
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(VMSTOP_DISKFULL);
+        vm_stop(QSTATE_IOERROR);
     } else {
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
             scsi_req_data(&r->req, 0);
@@ -333,7 +333,7 @@ static void scsi_dma_restart_bh(void *opaque)
     }
 }
 
-static void scsi_dma_restart_cb(void *opaque, int running, int reason)
+static void scsi_dma_restart_cb(void *opaque, int running, QemuState state)
 {
     SCSIDiskState *s = opaque;
 
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 6471ac8..48b6aa7 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -78,7 +78,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         req->next = s->rq;
         s->rq = req;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
-        vm_stop(VMSTOP_DISKFULL);
+        vm_stop(QSTATE_IOERROR);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
@@ -423,7 +423,8 @@ static void virtio_blk_dma_restart_bh(void *opaque)
     virtio_submit_multiwrite(s->bs, &mrb);
 }
 
-static void virtio_blk_dma_restart_cb(void *opaque, int running, int reason)
+static void virtio_blk_dma_restart_cb(void *opaque, int running,
+                                      QemuState state)
 {
     VirtIOBlock *s = opaque;
 
diff --git a/hw/virtio.c b/hw/virtio.c
index a8f4940..7293db0 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -836,7 +836,7 @@ void virtio_cleanup(VirtIODevice *vdev)
     qemu_free(vdev->vq);
 }
 
-static void virtio_vmstate_change(void *opaque, int running, int reason)
+static void virtio_vmstate_change(void *opaque, int running, QemuState state)
 {
     VirtIODevice *vdev = opaque;
     bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK);
diff --git a/hw/watchdog.c b/hw/watchdog.c
index 1c900a1..a3c2d9b 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -132,7 +132,7 @@ void watchdog_perform_action(void)
 
     case WDT_PAUSE:             /* same as 'stop' command in monitor */
         watchdog_mon_event("pause");
-        vm_stop(VMSTOP_WATCHDOG);
+        vm_stop(QSTATE_WATCHDOG);
         break;
 
     case WDT_DEBUG:
diff --git a/kvm-all.c b/kvm-all.c
index cbc2532..4da5fff 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1014,7 +1014,7 @@ int kvm_cpu_exec(CPUState *env)
 
     if (ret < 0) {
         cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
-        vm_stop(VMSTOP_PANIC);
+        vm_stop(QSTATE_INTERROR);
     }
 
     env->exit_request = 0;
diff --git a/migration.c b/migration.c
index 2a15b98..9724ce0 100644
--- a/migration.c
+++ b/migration.c
@@ -378,7 +378,7 @@ void migrate_fd_put_ready(void *opaque)
         int old_vm_running = vm_running;
 
         DPRINTF("done iterating\n");
-        vm_stop(VMSTOP_MIGRATE);
+        vm_stop(QSTATE_PREMIGRATE);
 
         if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
             if (old_vm_running) {
diff --git a/monitor.c b/monitor.c
index 1b8ba2c..6ce0391 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1291,7 +1291,7 @@ static void do_singlestep(Monitor *mon, const QDict *qdict)
  */
 static int do_stop(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    vm_stop(VMSTOP_USER);
+    vm_stop(QSTATE_PAUSED);
     return 0;
 }
 
@@ -2814,7 +2814,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
     int saved_vm_running  = vm_running;
     const char *name = qdict_get_str(qdict, "name");
 
-    vm_stop(VMSTOP_LOADVM);
+    vm_stop(QSTATE_RESTVM);
 
     if (load_vmstate(name) == 0 && saved_vm_running) {
         vm_start();
diff --git a/qemu-timer.c b/qemu-timer.c
index 30e8f12..5dbaaa3 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -1134,7 +1134,8 @@ static void win32_rearm_timer(struct qemu_alarm_timer *t)
 
 #endif /* _WIN32 */
 
-static void alarm_timer_on_change_state_rearm(void *opaque, int running, int reason)
+static void alarm_timer_on_change_state_rearm(void *opaque, int running,
+                                              QemuState state)
 {
     if (running)
         qemu_rearm_alarm_timer((struct qemu_alarm_timer *) opaque);
diff --git a/savevm.c b/savevm.c
index 7801aa7..d8db493 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1603,7 +1603,7 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     int ret;
 
     saved_vm_running = vm_running;
-    vm_stop(VMSTOP_SAVEVM);
+    vm_stop(QSTATE_SAVEVM);
 
     if (qemu_savevm_state_blocked(mon)) {
         ret = -EINVAL;
@@ -1932,7 +1932,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     saved_vm_running = vm_running;
-    vm_stop(VMSTOP_SAVEVM);
+    vm_stop(QSTATE_SAVEVM);
 
     memset(sn, 0, sizeof(*sn));
 
diff --git a/sysemu.h b/sysemu.h
index 61ad4fb..2c3ea3d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -9,6 +9,20 @@
 #include "notify.h"
 
 /* vl.c */
+
+typedef enum {
+    QSTATE_DEBUG,         /* qemu is running under gdb */
+    QSTATE_INTERROR,      /* paused due to an internal error */
+    QSTATE_IOERROR,       /* paused due to an I/O error */
+    QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
+    QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
+    QSTATE_RESTVM,        /* paused restoring the VM state */
+    QSTATE_RUNNING,       /* qemu is running */
+    QSTATE_SAVEVM,        /* paused saving VM state */
+    QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in use */
+    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to pause */
+} QemuState;
+
 extern const char *bios_name;
 
 extern int vm_running;
@@ -18,28 +32,18 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
 typedef struct vm_change_state_entry VMChangeStateEntry;
-typedef void VMChangeStateHandler(void *opaque, int running, int reason);
+typedef void VMChangeStateHandler(void *opaque, int running, QemuState state);
 
 VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb,
                                                      void *opaque);
 void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
-void vm_state_notify(int running, int reason);
-
-#define VMSTOP_USER      0
-#define VMSTOP_DEBUG     1
-#define VMSTOP_SHUTDOWN  2
-#define VMSTOP_DISKFULL  3
-#define VMSTOP_WATCHDOG  4
-#define VMSTOP_PANIC     5
-#define VMSTOP_SAVEVM    6
-#define VMSTOP_LOADVM    7
-#define VMSTOP_MIGRATE   8
+void vm_state_notify(int running, QemuState state);
 
 #define VMRESET_SILENT   false
 #define VMRESET_REPORT   true
 
 void vm_start(void);
-void vm_stop(int reason);
+void vm_stop(QemuState state);
 
 void qemu_system_reset_request(void);
 void qemu_system_shutdown_request(void);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 10fb2c4..c88cd14 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -334,7 +334,7 @@ static int kvm_inject_mce_oldstyle(CPUState *env)
     return 0;
 }
 
-static void cpu_update_state(void *opaque, int running, int reason)
+static void cpu_update_state(void *opaque, int running, QemuState state)
 {
     CPUState *env = opaque;
 
diff --git a/ui/spice-display.c b/ui/spice-display.c
index feeee73..02baeaf 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -191,7 +191,8 @@ void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
     ssd->worker->destroy_primary_surface(ssd->worker, 0);
 }
 
-void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
+void qemu_spice_vm_change_state_handler(void *opaque, int running,
+                                        QemuState state)
 {
     SimpleSpiceDisplay *ssd = opaque;
 
diff --git a/vl.c b/vl.c
index 426cea7..faa7c5f 100644
--- a/vl.c
+++ b/vl.c
@@ -1142,14 +1142,14 @@ void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
     qemu_free (e);
 }
 
-void vm_state_notify(int running, int reason)
+void vm_state_notify(int running, QemuState state)
 {
     VMChangeStateEntry *e;
 
-    trace_vm_state_notify(running, reason);
+    trace_vm_state_notify(running, state);
 
     for (e = vm_change_state_head.lh_first; e; e = e->entries.le_next) {
-        e->cb(e->opaque, running, reason);
+        e->cb(e->opaque, running, state);
     }
 }
 
@@ -1158,7 +1158,7 @@ void vm_start(void)
     if (!vm_running) {
         cpu_enable_ticks();
         vm_running = 1;
-        vm_state_notify(1, 0);
+        vm_state_notify(1, QSTATE_RUNNING);
         resume_all_vcpus();
         monitor_protocol_event(QEVENT_RESUME, NULL);
     }
@@ -1402,13 +1402,13 @@ static void main_loop(void)
 #endif
 
         if (qemu_debug_requested()) {
-            vm_stop(VMSTOP_DEBUG);
+            vm_stop(QSTATE_DEBUG);
         }
         if (qemu_shutdown_requested()) {
             qemu_kill_report();
             monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
             if (no_shutdown) {
-                vm_stop(VMSTOP_SHUTDOWN);
+                vm_stop(QSTATE_SHUTDOWN);
             } else
                 break;
         }
diff --git a/xen-all.c b/xen-all.c
index 9eaeac1..5d2bc4c 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -846,7 +846,8 @@ static void xen_main_loop_prepare(XenIOState *state)
 
 /* Initialise Xen */
 
-static void xen_change_state_handler(void *opaque, int running, int reason)
+static void xen_change_state_handler(void *opaque, int running,
+                                     QemuState state)
 {
     if (running) {
         /* record state running */
@@ -854,7 +855,8 @@ static void xen_change_state_handler(void *opaque, int running, int reason)
     }
 }
 
-static void xen_hvm_change_state_handler(void *opaque, int running, int reason)
+static void xen_hvm_change_state_handler(void *opaque, int running,
+                                         QemuState state)
 {
     XenIOState *state = opaque;
     if (running) {
-- 
1.7.6.396.ge0613

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Avi Kivity]

On 08/03/2011 06:17 PM, Luiz Capitulino wrote:
> @@ -9,6 +9,20 @@
>   #include "notify.h"
>
>   /* vl.c */
> +
> +typedef enum {
> +    QSTATE_DEBUG,         /* qemu is running under gdb */
> +    QSTATE_INTERROR,      /* paused due to an internal error */
> +    QSTATE_IOERROR,       /* paused due to an I/O error */
> +    QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
> +    QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
> +    QSTATE_RESTVM,        /* paused restoring the VM state */
> +    QSTATE_RUNNING,       /* qemu is running */
> +    QSTATE_SAVEVM,        /* paused saving VM state */
> +    QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in use */
> +    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to pause */
> +} QemuState;
> +
>   extern const char *bios_name;
>

Why "QemuState"?  In general, "qemu" can be inferred from the fact that 
we're in qemu.git.  Suggest "RunState".

Second, these states can coexist.  A user may pause the VM 
simultaneously with the watchdog firing or entering premigrate state.  
In fact, with multiple monitors, each monitor can pause and resume the 
vm independently.

So I think we should keep a reference count instead of just a start/stop 
state.  Perhaps

vm_stop(QemuState s)
{
     ++stopcount[s];
}

vm_is_stopped()
{
     for (s in states)
           if (stopcount[s])
               return true;
     return false;
}

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Jan Kiszka]

On 2011-08-04 11:55, Avi Kivity wrote:
> On 08/03/2011 06:17 PM, Luiz Capitulino wrote:
>> @@ -9,6 +9,20 @@
>>   #include "notify.h"
>>
>>   /* vl.c */
>> +
>> +typedef enum {
>> +    QSTATE_DEBUG,         /* qemu is running under gdb */
>> +    QSTATE_INTERROR,      /* paused due to an internal error */
>> +    QSTATE_IOERROR,       /* paused due to an I/O error */
>> +    QSTATE_PAUSED,        /* paused by the user (ie. the 'stop'
>> command) */
>> +    QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
>> +    QSTATE_RESTVM,        /* paused restoring the VM state */
>> +    QSTATE_RUNNING,       /* qemu is running */
>> +    QSTATE_SAVEVM,        /* paused saving VM state */
>> +    QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in
>> use */
>> +    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to
>> pause */
>> +} QemuState;
>> +
>>   extern const char *bios_name;
>>
> 
> Why "QemuState"?  In general, "qemu" can be inferred from the fact that
> we're in qemu.git.  Suggest "RunState".
> 
> Second, these states can coexist.  A user may pause the VM
> simultaneously with the watchdog firing or entering premigrate state. 
> In fact, with multiple monitors, each monitor can pause and resume the
> vm independently.
> 
> So I think we should keep a reference count instead of just a start/stop
> state.  Perhaps
> 
> vm_stop(QemuState s)
> {
>     ++stopcount[s];
> }
> 
> vm_is_stopped()
> {
>     for (s in states)
>           if (stopcount[s])
>               return true;
>     return false;
> }

I don't think this makes sense nor is user-friendly. If one command
channel suspends the machine, others have the chance to subscribe for
that event. Maintaining a suspension counter would mean you also need a
channel to query its value.

IMHO, there is also no use for defining stopped orthogonally to
premigrate and other states that imply that the machine is stopped.
Basically they mean "stopped for/because of X". We just need to avoid
that you can enter plain stopped state from them by issuing the
corresponding monitor command. The other way around might be possible,
though, if there are race windows.

Jan


-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Jan Kiszka]

On 2011-08-04 12:17, Jan Kiszka wrote:
> On 2011-08-04 11:55, Avi Kivity wrote:
>> On 08/03/2011 06:17 PM, Luiz Capitulino wrote:
>>> @@ -9,6 +9,20 @@
>>>   #include "notify.h"
>>>
>>>   /* vl.c */
>>> +
>>> +typedef enum {
>>> +    QSTATE_DEBUG,         /* qemu is running under gdb */
>>> +    QSTATE_INTERROR,      /* paused due to an internal error */
>>> +    QSTATE_IOERROR,       /* paused due to an I/O error */
>>> +    QSTATE_PAUSED,        /* paused by the user (ie. the 'stop'
>>> command) */
>>> +    QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
>>> +    QSTATE_RESTVM,        /* paused restoring the VM state */
>>> +    QSTATE_RUNNING,       /* qemu is running */
>>> +    QSTATE_SAVEVM,        /* paused saving VM state */
>>> +    QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in
>>> use */
>>> +    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to
>>> pause */
>>> +} QemuState;
>>> +
>>>   extern const char *bios_name;
>>>
>>
>> Why "QemuState"?  In general, "qemu" can be inferred from the fact that
>> we're in qemu.git.  Suggest "RunState".
>>
>> Second, these states can coexist.  A user may pause the VM
>> simultaneously with the watchdog firing or entering premigrate state. 
>> In fact, with multiple monitors, each monitor can pause and resume the
>> vm independently.
>>
>> So I think we should keep a reference count instead of just a start/stop
>> state.  Perhaps
>>
>> vm_stop(QemuState s)
>> {
>>     ++stopcount[s];
>> }
>>
>> vm_is_stopped()
>> {
>>     for (s in states)
>>           if (stopcount[s])
>>               return true;
>>     return false;
>> }
> 
> I don't think this makes sense nor is user-friendly. If one command
> channel suspends the machine, others have the chance to subscribe for
> that event. Maintaining a suspension counter would mean you also need a
> channel to query its value.
> 
> IMHO, there is also no use for defining stopped orthogonally to
> premigrate and other states that imply that the machine is stopped.
> Basically they mean "stopped for/because of X". We just need to avoid
> that you can enter plain stopped state from them by issuing the
> corresponding monitor command. The other way around might be possible,
> though, if there are race windows.

The makes me wonder if qemu_state_set shouldn't validate if the state
transition is legal (simple switch/case). That way, we would have a
central point and could also avoid potential races or logical bugs where
a newly set state is accidentally overwritten due to unexpected
execution order.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

On Thu, 04 Aug 2011 12:17:55 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2011-08-04 11:55, Avi Kivity wrote:
> > On 08/03/2011 06:17 PM, Luiz Capitulino wrote:
> >> @@ -9,6 +9,20 @@
> >>   #include "notify.h"
> >>
> >>   /* vl.c */
> >> +
> >> +typedef enum {
> >> +    QSTATE_DEBUG,         /* qemu is running under gdb */
> >> +    QSTATE_INTERROR,      /* paused due to an internal error */
> >> +    QSTATE_IOERROR,       /* paused due to an I/O error */
> >> +    QSTATE_PAUSED,        /* paused by the user (ie. the 'stop'
> >> command) */
> >> +    QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
> >> +    QSTATE_RESTVM,        /* paused restoring the VM state */
> >> +    QSTATE_RUNNING,       /* qemu is running */
> >> +    QSTATE_SAVEVM,        /* paused saving VM state */
> >> +    QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in
> >> use */
> >> +    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to
> >> pause */
> >> +} QemuState;
> >> +
> >>   extern const char *bios_name;
> >>
> > 
> > Why "QemuState"?  In general, "qemu" can be inferred from the fact that
> > we're in qemu.git.  Suggest "RunState".

Having a RUNSTATE_PAUSED seems a bit strange when reading it for the
first time. But I chose QemuState because I didn't have other options,
so I'm fine with RunState.

> > Second, these states can coexist.  A user may pause the VM
> > simultaneously with the watchdog firing or entering premigrate state. 
> > In fact, with multiple monitors, each monitor can pause and resume the
> > vm independently.
> > 
> > So I think we should keep a reference count instead of just a start/stop
> > state.  Perhaps
> > 
> > vm_stop(QemuState s)
> > {
> >     ++stopcount[s];
> > }
> > 
> > vm_is_stopped()
> > {
> >     for (s in states)
> >           if (stopcount[s])
> >               return true;
> >     return false;
> > }
> 
> I don't think this makes sense nor is user-friendly. If one command
> channel suspends the machine, others have the chance to subscribe for
> that event. Maintaining a suspension counter would mean you also need a
> channel to query its value.

Agreed. Can be done incrementally if this turns out to be needed.

> 
> IMHO, there is also no use for defining stopped orthogonally to
> premigrate and other states that imply that the machine is stopped.
> Basically they mean "stopped for/because of X". We just need to avoid
> that you can enter plain stopped state from them by issuing the
> corresponding monitor command. The other way around might be possible,
> though, if there are race windows.
> 
> Jan
> 
> 

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Avi Kivity]

On 08/04/2011 01:17 PM, Jan Kiszka wrote:
> >
> >  Why "QemuState"?  In general, "qemu" can be inferred from the fact that
> >  we're in qemu.git.  Suggest "RunState".
> >
> >  Second, these states can coexist.  A user may pause the VM
> >  simultaneously with the watchdog firing or entering premigrate state.
> >  In fact, with multiple monitors, each monitor can pause and resume the
> >  vm independently.
> >
> >  So I think we should keep a reference count instead of just a start/stop
> >  state.  Perhaps
> >
> >  vm_stop(QemuState s)
> >  {
> >      ++stopcount[s];
> >  }
> >
> >  vm_is_stopped()
> >  {
> >      for (s in states)
> >            if (stopcount[s])
> >                return true;
> >      return false;
> >  }
>
> I don't think this makes sense nor is user-friendly. If one command
> channel suspends the machine, others have the chance to subscribe for
> that event.

It's inherently racy.

> Maintaining a suspension counter would mean you also need a
> channel to query its value.

Why?

> IMHO, there is also no use for defining stopped orthogonally to
> premigrate and other states that imply that the machine is stopped.
> Basically they mean "stopped for/because of X". We just need to avoid
> that you can enter plain stopped state from them by issuing the
> corresponding monitor command. The other way around might be possible,
> though, if there are race windows.
>

I'm worried about the following race:

   stop
   (qemu stopped for internal reason)
   stop comment processed

   resume

The (qemu stopped for internal reason) part is lost.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

On Mon, 08 Aug 2011 14:22:14 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/04/2011 01:17 PM, Jan Kiszka wrote:
> > >
> > >  Why "QemuState"?  In general, "qemu" can be inferred from the fact that
> > >  we're in qemu.git.  Suggest "RunState".
> > >
> > >  Second, these states can coexist.  A user may pause the VM
> > >  simultaneously with the watchdog firing or entering premigrate state.
> > >  In fact, with multiple monitors, each monitor can pause and resume the
> > >  vm independently.
> > >
> > >  So I think we should keep a reference count instead of just a start/stop
> > >  state.  Perhaps
> > >
> > >  vm_stop(QemuState s)
> > >  {
> > >      ++stopcount[s];
> > >  }
> > >
> > >  vm_is_stopped()
> > >  {
> > >      for (s in states)
> > >            if (stopcount[s])
> > >                return true;
> > >      return false;
> > >  }
> >
> > I don't think this makes sense nor is user-friendly. If one command
> > channel suspends the machine, others have the chance to subscribe for
> > that event.
> 
> It's inherently racy.
> 
> > Maintaining a suspension counter would mean you also need a
> > channel to query its value.
> 
> Why?
> 
> > IMHO, there is also no use for defining stopped orthogonally to
> > premigrate and other states that imply that the machine is stopped.
> > Basically they mean "stopped for/because of X". We just need to avoid
> > that you can enter plain stopped state from them by issuing the
> > corresponding monitor command. The other way around might be possible,
> > though, if there are race windows.
> >
> 
> I'm worried about the following race:
> 
>    stop
>    (qemu stopped for internal reason)
>    stop comment processed
> 
>    resume
> 
> The (qemu stopped for internal reason) part is lost.

If the "stop" you're referring to happens through vm_stop(), then no,
it won't be lost because do_vm_stop() doesn't allow qemu to be stopped
twice.

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Avi Kivity]

On 08/08/2011 04:25 PM, Luiz Capitulino wrote:
> >
> >  I'm worried about the following race:
> >
> >     stop
> >     (qemu stopped for internal reason)
> >     stop comment processed
> >
> >     resume
> >
> >  The (qemu stopped for internal reason) part is lost.
>
> If the "stop" you're referring to happens through vm_stop(), then no,
> it won't be lost because do_vm_stop() doesn't allow qemu to be stopped
> twice.

What happens then?  The user sees an error?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

On Mon, 08 Aug 2011 16:27:05 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/08/2011 04:25 PM, Luiz Capitulino wrote:
> > >
> > >  I'm worried about the following race:
> > >
> > >     stop
> > >     (qemu stopped for internal reason)
> > >     stop comment processed
> > >
> > >     resume
> > >
> > >  The (qemu stopped for internal reason) part is lost.
> >
> > If the "stop" you're referring to happens through vm_stop(), then no,
> > it won't be lost because do_vm_stop() doesn't allow qemu to be stopped
> > twice.
> 
> What happens then?  The user sees an error?

It's ignored.

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Avi Kivity]

On 08/08/2011 04:28 PM, Luiz Capitulino wrote:
> On Mon, 08 Aug 2011 16:27:05 +0300
> Avi Kivity<avi@redhat.com>  wrote:
>
> >  On 08/08/2011 04:25 PM, Luiz Capitulino wrote:
> >  >  >
> >  >  >   I'm worried about the following race:
> >  >  >
> >  >  >      stop
> >  >  >      (qemu stopped for internal reason)
> >  >  >      stop comment processed
> >  >  >
> >  >  >      resume
> >  >  >
> >  >  >   The (qemu stopped for internal reason) part is lost.
> >  >
> >  >  If the "stop" you're referring to happens through vm_stop(), then no,
> >  >  it won't be lost because do_vm_stop() doesn't allow qemu to be stopped
> >  >  twice.
> >
> >  What happens then?  The user sees an error?
>
> It's ignored.

Well, then, the user won't know something happened and will happily 
resume the guest, like I outlined above.

When you ignore something in the first set, something breaks in the third.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

On Mon, 08 Aug 2011 16:40:17 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/08/2011 04:28 PM, Luiz Capitulino wrote:
> > On Mon, 08 Aug 2011 16:27:05 +0300
> > Avi Kivity<avi@redhat.com>  wrote:
> >
> > >  On 08/08/2011 04:25 PM, Luiz Capitulino wrote:
> > >  >  >
> > >  >  >   I'm worried about the following race:
> > >  >  >
> > >  >  >      stop
> > >  >  >      (qemu stopped for internal reason)
> > >  >  >      stop comment processed
> > >  >  >
> > >  >  >      resume
> > >  >  >
> > >  >  >   The (qemu stopped for internal reason) part is lost.
> > >  >
> > >  >  If the "stop" you're referring to happens through vm_stop(), then no,
> > >  >  it won't be lost because do_vm_stop() doesn't allow qemu to be stopped
> > >  >  twice.
> > >
> > >  What happens then?  The user sees an error?
> >
> > It's ignored.
> 
> Well, then, the user won't know something happened and will happily 
> resume the guest, like I outlined above.

I think it makes sense to return an error in the monitor if the user
tries to stop qemu when it's already stopped. Not sure if it will do what you
think it should do, but we should always tell the user when we're unable to
carry his/her orders.

But it does make sense to me to not allow stopping twice. First because it
doesn't make sense to stop something which is not moving and second because
what else can stop the vm if it's already stopped?

Maybe vm_stop() should return an error, but I think this goes beyond this
series.

> 
> When you ignore something in the first set, something breaks in the third.
> 

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Avi Kivity]

On 08/08/2011 04:47 PM, Luiz Capitulino wrote:
> >
> >  Well, then, the user won't know something happened and will happily
> >  resume the guest, like I outlined above.
>
> I think it makes sense to return an error in the monitor if the user
> tries to stop qemu when it's already stopped. Not sure if it will do what you
> think it should do, but we should always tell the user when we're unable to
> carry his/her orders.
>
> But it does make sense to me to not allow stopping twice. First because it
> doesn't make sense to stop something which is not moving and second because
> what else can stop the vm if it's already stopped?
>
> Maybe vm_stop() should return an error, but I think this goes beyond this
> series.
>

This is why I suggested a reference count.  In this case, we can always 
stop the guest "twice", because we don't lost information when we resume.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

On Mon, 08 Aug 2011 16:54:16 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/08/2011 04:47 PM, Luiz Capitulino wrote:
> > >
> > >  Well, then, the user won't know something happened and will happily
> > >  resume the guest, like I outlined above.
> >
> > I think it makes sense to return an error in the monitor if the user
> > tries to stop qemu when it's already stopped. Not sure if it will do what you
> > think it should do, but we should always tell the user when we're unable to
> > carry his/her orders.
> >
> > But it does make sense to me to not allow stopping twice. First because it
> > doesn't make sense to stop something which is not moving and second because
> > what else can stop the vm if it's already stopped?
> >
> > Maybe vm_stop() should return an error, but I think this goes beyond this
> > series.
> >
> 
> This is why I suggested a reference count.  In this case, we can always 
> stop the guest "twice", because we don't lost information when we resume.

I could give it a try in the near future, as I really think it's independent
from this series, but I still don't understand what can stop an already stopped
VM besides the user. This is a real question, is it really possible?

If only the user can do that, then the refcount is overkill as just returning
an error will do it.

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Avi Kivity]

On 08/08/2011 05:06 PM, Luiz Capitulino wrote:
> >
> >  This is why I suggested a reference count.  In this case, we can always
> >  stop the guest "twice", because we don't lost information when we resume.
>
> I could give it a try in the near future, as I really think it's independent
> from this series, but I still don't understand what can stop an already stopped
> VM besides the user. This is a real question, is it really possible?
>
> If only the user can do that, then the refcount is overkill as just returning
> an error will do it.

Most of the reasons in QemuState are asynchronous and can happen at any 
time while the guest is running.  Because they're asynchronous, they can 
trigger before a monitor stop is issued but caught after it is processed.

We could possibly synchronize during user stop, but that lets us capture 
only the first non-user reason.

And even if we return an error, that only pushes the refcounting to the 
user; you probably don't want to return a "vm is already stopped" error 
to the user, he'll just ask "why are you telling me that".

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/7] Replace VMSTOP macros with a proper QemuState type

[Luiz Capitulino]

On Mon, 08 Aug 2011 17:27:24 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 08/08/2011 05:06 PM, Luiz Capitulino wrote:
> > >
> > >  This is why I suggested a reference count.  In this case, we can always
> > >  stop the guest "twice", because we don't lost information when we resume.
> >
> > I could give it a try in the near future, as I really think it's independent
> > from this series, but I still don't understand what can stop an already stopped
> > VM besides the user. This is a real question, is it really possible?
> >
> > If only the user can do that, then the refcount is overkill as just returning
> > an error will do it.
> 
> Most of the reasons in QemuState are asynchronous and can happen at any 
> time while the guest is running.  Because they're asynchronous, they can 
> trigger before a monitor stop is issued but caught after it is processed.
> 
> We could possibly synchronize during user stop, but that lets us capture 
> only the first non-user reason.

Isn't it good enough?

Another question: do you think that this problem is so relevant that we
should include the fix in the first merge?

> 
> And even if we return an error, that only pushes the refcounting to the 
> user; you probably don't want to return a "vm is already stopped" error 
> to the user, he'll just ask "why are you telling me that".
> 

[Qemu-devel] [PATCH 3/7] QemuState: Add additional states

[Luiz Capitulino]

Currently, only vm_start() and vm_stop() change the VM state. That's,
the state is only changed when starting or stopping the VM.

This commit adds the qemu_state_set() function, making it possible
to also do state transitions when qemu is stopped or running.

Additional states are also added and the current state is stored.
This is going to be used by the next commits.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cpus.c      |    1 +
 migration.c |    8 +++++++-
 sysemu.h    |   10 +++++++++-
 vl.c        |   20 ++++++++++++++++++++
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index ebbb8b9..48e6ca1 100644
--- a/cpus.c
+++ b/cpus.c
@@ -124,6 +124,7 @@ static void do_vm_stop(QemuState state)
         cpu_disable_ticks();
         vm_running = 0;
         pause_all_vcpus();
+        qemu_state_set(state);
         vm_state_notify(0, state);
         qemu_aio_flush();
         bdrv_flush_all();
diff --git a/migration.c b/migration.c
index 9724ce0..8aacf64 100644
--- a/migration.c
+++ b/migration.c
@@ -72,8 +72,11 @@ void process_incoming_migration(QEMUFile *f)
 
     incoming_expected = false;
 
-    if (autostart)
+    if (autostart) {
         vm_start();
+    } else {
+        qemu_state_set(QSTATE_PRELAUNCH);
+    }
 }
 
 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
@@ -394,6 +397,9 @@ void migrate_fd_put_ready(void *opaque)
             }
             state = MIG_STATE_ERROR;
         }
+        if (state == MIG_STATE_COMPLETED) {
+            qemu_state_set(QSTATE_POSTMIGRATE);
+        }
         s->state = state;
         notifier_list_notify(&migration_state_notifiers, NULL);
     }
diff --git a/sysemu.h b/sysemu.h
index 2c3ea3d..32c9abb 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -11,16 +11,22 @@
 /* vl.c */
 
 typedef enum {
+    QSTATE_NOSTATE,
     QSTATE_DEBUG,         /* qemu is running under gdb */
+    QSTATE_INMIGRATE,     /* paused waiting for an incoming migration */
     QSTATE_INTERROR,      /* paused due to an internal error */
     QSTATE_IOERROR,       /* paused due to an I/O error */
     QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
+    QSTATE_POSTMIGRATE,   /* paused following a successful migration */
+    QSTATE_PRELAUNCH,     /* qemu was started with -S and haven't started */
     QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
     QSTATE_RESTVM,        /* paused restoring the VM state */
+    QSTATE_RESTVMFAILED,  /* paused due to a failed attempt to load state */
     QSTATE_RUNNING,       /* qemu is running */
     QSTATE_SAVEVM,        /* paused saving VM state */
     QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in use */
-    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to pause */
+    QSTATE_WATCHDOG,      /* watchdog fired and qemu is configured to pause */
+    QSTATE_MAX
 } QemuState;
 
 extern const char *bios_name;
@@ -31,6 +37,8 @@ extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
+QemuState qemu_state_get(void);
+void qemu_state_set(QemuState state);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, QemuState state);
 
diff --git a/vl.c b/vl.c
index faa7c5f..2619c8e 100644
--- a/vl.c
+++ b/vl.c
@@ -320,6 +320,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
 }
 
 /***********************************************************/
+/* QEMU state */
+
+static QemuState qemu_current_state = QSTATE_NOSTATE;
+
+QemuState qemu_state_get(void)
+{
+    return qemu_current_state;
+}
+
+void qemu_state_set(QemuState state)
+{
+    assert(state < QSTATE_MAX);
+    qemu_current_state = state;
+}
+
+/***********************************************************/
 /* real time host monotonic timer */
 
 /***********************************************************/
@@ -1158,6 +1174,7 @@ void vm_start(void)
     if (!vm_running) {
         cpu_enable_ticks();
         vm_running = 1;
+        qemu_state_set(QSTATE_RUNNING);
         vm_state_notify(1, QSTATE_RUNNING);
         resume_all_vcpus();
         monitor_protocol_event(QEVENT_RESUME, NULL);
@@ -3341,6 +3358,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (incoming) {
+        qemu_state_set(QSTATE_INMIGRATE);
         int ret = qemu_start_incoming_migration(incoming);
         if (ret < 0) {
             fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
@@ -3349,6 +3367,8 @@ int main(int argc, char **argv, char **envp)
         }
     } else if (autostart) {
         vm_start();
+    } else {
+        qemu_state_set(QSTATE_PRELAUNCH);
     }
 
     os_setup_post();
-- 
1.7.6.396.ge0613

Re: [Qemu-devel] [PATCH 3/7] QemuState: Add additional states

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Currently, only vm_start() and vm_stop() change the VM state. That's,
> the state is only changed when starting or stopping the VM.
>
> This commit adds the qemu_state_set() function, making it possible
> to also do state transitions when qemu is stopped or running.
>
> Additional states are also added and the current state is stored.
> This is going to be used by the next commits.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  cpus.c      |    1 +
>  migration.c |    8 +++++++-
>  sysemu.h    |   10 +++++++++-
>  vl.c        |   20 ++++++++++++++++++++
>  4 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index ebbb8b9..48e6ca1 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -124,6 +124,7 @@ static void do_vm_stop(QemuState state)
>          cpu_disable_ticks();
>          vm_running = 0;
>          pause_all_vcpus();
> +        qemu_state_set(state);
>          vm_state_notify(0, state);
>          qemu_aio_flush();
>          bdrv_flush_all();
> diff --git a/migration.c b/migration.c
> index 9724ce0..8aacf64 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -72,8 +72,11 @@ void process_incoming_migration(QEMUFile *f)
>  
>      incoming_expected = false;
>  
> -    if (autostart)
> +    if (autostart) {
>          vm_start();
> +    } else {
> +        qemu_state_set(QSTATE_PRELAUNCH);
> +    }
>  }
>  
>  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> @@ -394,6 +397,9 @@ void migrate_fd_put_ready(void *opaque)
>              }
>              state = MIG_STATE_ERROR;
>          }
> +        if (state == MIG_STATE_COMPLETED) {
> +            qemu_state_set(QSTATE_POSTMIGRATE);
> +        }
>          s->state = state;
>          notifier_list_notify(&migration_state_notifiers, NULL);
>      }
> diff --git a/sysemu.h b/sysemu.h
> index 2c3ea3d..32c9abb 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -11,16 +11,22 @@
>  /* vl.c */
>  
>  typedef enum {
> +    QSTATE_NOSTATE,

QSTATE_NO_STATE?

>      QSTATE_DEBUG,         /* qemu is running under gdb */
> +    QSTATE_INMIGRATE,     /* paused waiting for an incoming migration */

QSTATE_IN_MIGRATE?

>      QSTATE_INTERROR,      /* paused due to an internal error */

QSTATE_PANICKED?

>      QSTATE_IOERROR,       /* paused due to an I/O error */

QSTATE_IO_ERROR?

>      QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
> +    QSTATE_POSTMIGRATE,   /* paused following a successful migration */

QSTATE_POST_MIGRATE?

> +    QSTATE_PRELAUNCH,     /* qemu was started with -S and haven't started */

QSTATE_PRE_LAUNCH?

>      QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */

QSTATE_PRE_MIGRATE?

>      QSTATE_RESTVM,        /* paused restoring the VM state */
> +    QSTATE_RESTVMFAILED,  /* paused due to a failed attempt to load state */

QSTATE_RESTVM_FAILED?

Consistently separating words by spaces became a general custom about
the tenth century A.D., and lasted until about 1957, when FORTRAN
abandoned the practice.
	-- Sun FORTRAN Reference Manual

>      QSTATE_RUNNING,       /* qemu is running */
>      QSTATE_SAVEVM,        /* paused saving VM state */
>      QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in use */
> -    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to pause */
> +    QSTATE_WATCHDOG,      /* watchdog fired and qemu is configured to pause */
> +    QSTATE_MAX
>  } QemuState;
>  
>  extern const char *bios_name;
> @@ -31,6 +37,8 @@ extern uint8_t qemu_uuid[];
>  int qemu_uuid_parse(const char *str, uint8_t *uuid);
>  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
>  
> +QemuState qemu_state_get(void);
> +void qemu_state_set(QemuState state);
>  typedef struct vm_change_state_entry VMChangeStateEntry;
>  typedef void VMChangeStateHandler(void *opaque, int running, QemuState state);
>  
> diff --git a/vl.c b/vl.c
> index faa7c5f..2619c8e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -320,6 +320,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>  }
>  
>  /***********************************************************/
> +/* QEMU state */
> +
> +static QemuState qemu_current_state = QSTATE_NOSTATE;
> +
> +QemuState qemu_state_get(void)
> +{
> +    return qemu_current_state;
> +}
> +
> +void qemu_state_set(QemuState state)
> +{
> +    assert(state < QSTATE_MAX);

Beware, comparison is signed if QemuState is signed (implementation
defined; QSTATE_MAX is int).

> +    qemu_current_state = state;
> +}
> +
> +/***********************************************************/
[...]

Re: [Qemu-devel] [PATCH 3/7] QemuState: Add additional states

[Kevin Wolf]

Am 04.08.2011 11:02, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> Currently, only vm_start() and vm_stop() change the VM state. That's,
>> the state is only changed when starting or stopping the VM.
>>
>> This commit adds the qemu_state_set() function, making it possible
>> to also do state transitions when qemu is stopped or running.
>>
>> Additional states are also added and the current state is stored.
>> This is going to be used by the next commits.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  cpus.c      |    1 +
>>  migration.c |    8 +++++++-
>>  sysemu.h    |   10 +++++++++-
>>  vl.c        |   20 ++++++++++++++++++++
>>  4 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index ebbb8b9..48e6ca1 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -124,6 +124,7 @@ static void do_vm_stop(QemuState state)
>>          cpu_disable_ticks();
>>          vm_running = 0;
>>          pause_all_vcpus();
>> +        qemu_state_set(state);
>>          vm_state_notify(0, state);
>>          qemu_aio_flush();
>>          bdrv_flush_all();
>> diff --git a/migration.c b/migration.c
>> index 9724ce0..8aacf64 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -72,8 +72,11 @@ void process_incoming_migration(QEMUFile *f)
>>  
>>      incoming_expected = false;
>>  
>> -    if (autostart)
>> +    if (autostart) {
>>          vm_start();
>> +    } else {
>> +        qemu_state_set(QSTATE_PRELAUNCH);
>> +    }
>>  }
>>  
>>  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>> @@ -394,6 +397,9 @@ void migrate_fd_put_ready(void *opaque)
>>              }
>>              state = MIG_STATE_ERROR;
>>          }
>> +        if (state == MIG_STATE_COMPLETED) {
>> +            qemu_state_set(QSTATE_POSTMIGRATE);
>> +        }
>>          s->state = state;
>>          notifier_list_notify(&migration_state_notifiers, NULL);
>>      }
>> diff --git a/sysemu.h b/sysemu.h
>> index 2c3ea3d..32c9abb 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -11,16 +11,22 @@
>>  /* vl.c */
>>  
>>  typedef enum {
>> +    QSTATE_NOSTATE,
> 
> QSTATE_NO_STATE?
> 
>>      QSTATE_DEBUG,         /* qemu is running under gdb */
>> +    QSTATE_INMIGRATE,     /* paused waiting for an incoming migration */
> 
> QSTATE_IN_MIGRATE?
> 
>>      QSTATE_INTERROR,      /* paused due to an internal error */
> 
> QSTATE_PANICKED?
> 
>>      QSTATE_IOERROR,       /* paused due to an I/O error */
> 
> QSTATE_IO_ERROR?
> 
>>      QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
>> +    QSTATE_POSTMIGRATE,   /* paused following a successful migration */
> 
> QSTATE_POST_MIGRATE?
> 
>> +    QSTATE_PRELAUNCH,     /* qemu was started with -S and haven't started */
> 
> QSTATE_PRE_LAUNCH?
> 
>>      QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
> 
> QSTATE_PRE_MIGRATE?
> 
>>      QSTATE_RESTVM,        /* paused restoring the VM state */
>> +    QSTATE_RESTVMFAILED,  /* paused due to a failed attempt to load state */
> 
> QSTATE_RESTVM_FAILED?

QSTATE_RESTORE_FAILED?

It's hard to interpret RESTVM without having read the comment.

Kevin

Re: [Qemu-devel] [PATCH 3/7] QemuState: Add additional states

[Anthony Liguori]

On 08/04/2011 07:32 AM, Kevin Wolf wrote:
> Am 04.08.2011 11:02, schrieb Markus Armbruster:
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>> Currently, only vm_start() and vm_stop() change the VM state. That's,
>>> the state is only changed when starting or stopping the VM.
>>>
>>> This commit adds the qemu_state_set() function, making it possible
>>> to also do state transitions when qemu is stopped or running.
>>>
>>> Additional states are also added and the current state is stored.
>>> This is going to be used by the next commits.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>>   cpus.c      |    1 +
>>>   migration.c |    8 +++++++-
>>>   sysemu.h    |   10 +++++++++-
>>>   vl.c        |   20 ++++++++++++++++++++
>>>   4 files changed, 37 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index ebbb8b9..48e6ca1 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -124,6 +124,7 @@ static void do_vm_stop(QemuState state)
>>>           cpu_disable_ticks();
>>>           vm_running = 0;
>>>           pause_all_vcpus();
>>> +        qemu_state_set(state);
>>>           vm_state_notify(0, state);
>>>           qemu_aio_flush();
>>>           bdrv_flush_all();
>>> diff --git a/migration.c b/migration.c
>>> index 9724ce0..8aacf64 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -72,8 +72,11 @@ void process_incoming_migration(QEMUFile *f)
>>>
>>>       incoming_expected = false;
>>>
>>> -    if (autostart)
>>> +    if (autostart) {
>>>           vm_start();
>>> +    } else {
>>> +        qemu_state_set(QSTATE_PRELAUNCH);
>>> +    }
>>>   }
>>>
>>>   int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>> @@ -394,6 +397,9 @@ void migrate_fd_put_ready(void *opaque)
>>>               }
>>>               state = MIG_STATE_ERROR;
>>>           }
>>> +        if (state == MIG_STATE_COMPLETED) {
>>> +            qemu_state_set(QSTATE_POSTMIGRATE);
>>> +        }
>>>           s->state = state;
>>>           notifier_list_notify(&migration_state_notifiers, NULL);
>>>       }
>>> diff --git a/sysemu.h b/sysemu.h
>>> index 2c3ea3d..32c9abb 100644
>>> --- a/sysemu.h
>>> +++ b/sysemu.h
>>> @@ -11,16 +11,22 @@
>>>   /* vl.c */
>>>
>>>   typedef enum {
>>> +    QSTATE_NOSTATE,
>>
>> QSTATE_NO_STATE?
>>
>>>       QSTATE_DEBUG,         /* qemu is running under gdb */
>>> +    QSTATE_INMIGRATE,     /* paused waiting for an incoming migration */
>>
>> QSTATE_IN_MIGRATE?
>>
>>>       QSTATE_INTERROR,      /* paused due to an internal error */
>>
>> QSTATE_PANICKED?
>>
>>>       QSTATE_IOERROR,       /* paused due to an I/O error */
>>
>> QSTATE_IO_ERROR?
>>
>>>       QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
>>> +    QSTATE_POSTMIGRATE,   /* paused following a successful migration */
>>
>> QSTATE_POST_MIGRATE?
>>
>>> +    QSTATE_PRELAUNCH,     /* qemu was started with -S and haven't started */
>>
>> QSTATE_PRE_LAUNCH?
>>
>>>       QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
>>
>> QSTATE_PRE_MIGRATE?
>>
>>>       QSTATE_RESTVM,        /* paused restoring the VM state */
>>> +    QSTATE_RESTVMFAILED,  /* paused due to a failed attempt to load state */
>>
>> QSTATE_RESTVM_FAILED?
>
> QSTATE_RESTORE_FAILED?
>
> It's hard to interpret RESTVM without having read the comment.

Great suggestions!

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH 3/7] QemuState: Add additional states

[Luiz Capitulino]

On Thu, 04 Aug 2011 11:02:06 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Currently, only vm_start() and vm_stop() change the VM state. That's,
> > the state is only changed when starting or stopping the VM.
> >
> > This commit adds the qemu_state_set() function, making it possible
> > to also do state transitions when qemu is stopped or running.
> >
> > Additional states are also added and the current state is stored.
> > This is going to be used by the next commits.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  cpus.c      |    1 +
> >  migration.c |    8 +++++++-
> >  sysemu.h    |   10 +++++++++-
> >  vl.c        |   20 ++++++++++++++++++++
> >  4 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/cpus.c b/cpus.c
> > index ebbb8b9..48e6ca1 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -124,6 +124,7 @@ static void do_vm_stop(QemuState state)
> >          cpu_disable_ticks();
> >          vm_running = 0;
> >          pause_all_vcpus();
> > +        qemu_state_set(state);
> >          vm_state_notify(0, state);
> >          qemu_aio_flush();
> >          bdrv_flush_all();
> > diff --git a/migration.c b/migration.c
> > index 9724ce0..8aacf64 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -72,8 +72,11 @@ void process_incoming_migration(QEMUFile *f)
> >  
> >      incoming_expected = false;
> >  
> > -    if (autostart)
> > +    if (autostart) {
> >          vm_start();
> > +    } else {
> > +        qemu_state_set(QSTATE_PRELAUNCH);
> > +    }
> >  }
> >  
> >  int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > @@ -394,6 +397,9 @@ void migrate_fd_put_ready(void *opaque)
> >              }
> >              state = MIG_STATE_ERROR;
> >          }
> > +        if (state == MIG_STATE_COMPLETED) {
> > +            qemu_state_set(QSTATE_POSTMIGRATE);
> > +        }
> >          s->state = state;
> >          notifier_list_notify(&migration_state_notifiers, NULL);
> >      }
> > diff --git a/sysemu.h b/sysemu.h
> > index 2c3ea3d..32c9abb 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -11,16 +11,22 @@
> >  /* vl.c */
> >  
> >  typedef enum {
> > +    QSTATE_NOSTATE,
> 
> QSTATE_NO_STATE?
> 
> >      QSTATE_DEBUG,         /* qemu is running under gdb */
> > +    QSTATE_INMIGRATE,     /* paused waiting for an incoming migration */
> 
> QSTATE_IN_MIGRATE?
> 
> >      QSTATE_INTERROR,      /* paused due to an internal error */
> 
> QSTATE_PANICKED?
> 
> >      QSTATE_IOERROR,       /* paused due to an I/O error */
> 
> QSTATE_IO_ERROR?
> 
> >      QSTATE_PAUSED,        /* paused by the user (ie. the 'stop' command) */
> > +    QSTATE_POSTMIGRATE,   /* paused following a successful migration */
> 
> QSTATE_POST_MIGRATE?
> 
> > +    QSTATE_PRELAUNCH,     /* qemu was started with -S and haven't started */
> 
> QSTATE_PRE_LAUNCH?
> 
> >      QSTATE_PREMIGRATE,    /* paused preparing to finish migrate */
> 
> QSTATE_PRE_MIGRATE?
> 
> >      QSTATE_RESTVM,        /* paused restoring the VM state */
> > +    QSTATE_RESTVMFAILED,  /* paused due to a failed attempt to load state */
> 
> QSTATE_RESTVM_FAILED?
> 
> Consistently separating words by spaces became a general custom about
> the tenth century A.D., and lasted until about 1957, when FORTRAN
> abandoned the practice.
> 	-- Sun FORTRAN Reference Manual
> 
> >      QSTATE_RUNNING,       /* qemu is running */
> >      QSTATE_SAVEVM,        /* paused saving VM state */
> >      QSTATE_SHUTDOWN,      /* guest shut down and -no-shutdown is in use */
> > -    QSTATE_WATCHDOG       /* watchdog fired and qemu is configured to pause */
> > +    QSTATE_WATCHDOG,      /* watchdog fired and qemu is configured to pause */
> > +    QSTATE_MAX
> >  } QemuState;
> >  
> >  extern const char *bios_name;
> > @@ -31,6 +37,8 @@ extern uint8_t qemu_uuid[];
> >  int qemu_uuid_parse(const char *str, uint8_t *uuid);
> >  #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> >  
> > +QemuState qemu_state_get(void);
> > +void qemu_state_set(QemuState state);
> >  typedef struct vm_change_state_entry VMChangeStateEntry;
> >  typedef void VMChangeStateHandler(void *opaque, int running, QemuState state);
> >  
> > diff --git a/vl.c b/vl.c
> > index faa7c5f..2619c8e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -320,6 +320,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> >  }
> >  
> >  /***********************************************************/
> > +/* QEMU state */
> > +
> > +static QemuState qemu_current_state = QSTATE_NOSTATE;
> > +
> > +QemuState qemu_state_get(void)
> > +{
> > +    return qemu_current_state;
> > +}
> > +
> > +void qemu_state_set(QemuState state)
> > +{
> > +    assert(state < QSTATE_MAX);
> 
> Beware, comparison is signed if QemuState is signed (implementation
> defined; QSTATE_MAX is int).

It's unsigned here and I got the expected warning when I did:

 assert(state >= 0);

Don't how to address that (besides dropping the check).

> 
> > +    qemu_current_state = state;
> > +}
> > +
> > +/***********************************************************/
> [...]
> 

Re: [Qemu-devel] [PATCH 3/7] QemuState: Add additional states

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 04 Aug 2011 11:02:06 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Currently, only vm_start() and vm_stop() change the VM state. That's,
>> > the state is only changed when starting or stopping the VM.
>> >
>> > This commit adds the qemu_state_set() function, making it possible
>> > to also do state transitions when qemu is stopped or running.
>> >
>> > Additional states are also added and the current state is stored.
>> > This is going to be used by the next commits.
[...]
>> > diff --git a/vl.c b/vl.c
>> > index faa7c5f..2619c8e 100644
>> > --- a/vl.c
>> > +++ b/vl.c
>> > @@ -320,6 +320,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>> >  }
>> >  
>> >  /***********************************************************/
>> > +/* QEMU state */
>> > +
>> > +static QemuState qemu_current_state = QSTATE_NOSTATE;
>> > +
>> > +QemuState qemu_state_get(void)
>> > +{
>> > +    return qemu_current_state;
>> > +}
>> > +
>> > +void qemu_state_set(QemuState state)
>> > +{
>> > +    assert(state < QSTATE_MAX);
>> 
>> Beware, comparison is signed if QemuState is signed (implementation
>> defined; QSTATE_MAX is int).
>
> It's unsigned here and I got the expected warning when I did:
>
>  assert(state >= 0);
>
> Don't how to address that (besides dropping the check).

It's not likely to catch anthing the compiler doesn't.

If you want to check, and want to check thoroughly, then I'm afraid you
need to cast state.

>> > +    qemu_current_state = state;
>> > +}
>> > +
>> > +/***********************************************************/
>> [...]

[Qemu-devel] [PATCH 4/7] Drop the incoming_expected global variable

[Luiz Capitulino]

Test against QSTATE_INMIGRATE instead.

Please, note that the QSTATE_INMIGRATE state is only set when all the
initial VM setup is done, while 'incoming_expected' was set right in
the beginning when parsing command-line options. Shouldn't be a problem
as far as I could check.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 migration.c |    2 --
 monitor.c   |    2 +-
 vl.c        |    2 --
 3 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/migration.c b/migration.c
index 8aacf64..73e9bd6 100644
--- a/migration.c
+++ b/migration.c
@@ -70,8 +70,6 @@ void process_incoming_migration(QEMUFile *f)
     qemu_announce_self();
     DPRINTF("successfully loaded vm state\n");
 
-    incoming_expected = false;
-
     if (autostart) {
         vm_start();
     } else {
diff --git a/monitor.c b/monitor.c
index 6ce0391..05c5936 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1309,7 +1309,7 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     struct bdrv_iterate_context context = { mon, 0 };
 
-    if (incoming_expected) {
+    if (qemu_state_get() == QSTATE_INMIGRATE) {
         qerror_report(QERR_MIGRATION_EXPECTED);
         return -1;
     }
diff --git a/vl.c b/vl.c
index 2619c8e..c2ab7ce 100644
--- a/vl.c
+++ b/vl.c
@@ -185,7 +185,6 @@ int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int vm_running;
 int autostart;
-int incoming_expected; /* Started with -incoming and waiting for incoming */
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
 QEMUClock *rtc_clock;
@@ -2873,7 +2872,6 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
-                incoming_expected = true;
                 break;
             case QEMU_OPTION_nodefaults:
                 default_serial = 0;
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 5/7] Drop the vm_running global variable

[Luiz Capitulino]

Use qemu_is_running() instead, which is introduced by this commit and
is part of the QemuState API.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 cpus.c            |    9 ++++-----
 gdbstub.c         |    4 ++--
 hw/etraxfs_dma.c  |    2 +-
 hw/kvmclock.c     |    2 +-
 hw/virtio.c       |    2 +-
 migration.c       |    2 +-
 monitor.c         |    4 ++--
 qemu-timer.c      |    8 ++++----
 savevm.c          |    4 ++--
 sysemu.h          |    2 +-
 target-i386/kvm.c |    2 +-
 ui/sdl.c          |    6 +++---
 vl.c              |    9 ++++++---
 xen-all.c         |    2 +-
 14 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/cpus.c b/cpus.c
index 48e6ca1..65ea503 100644
--- a/cpus.c
+++ b/cpus.c
@@ -115,14 +115,13 @@ void cpu_synchronize_all_post_init(void)
 
 int cpu_is_stopped(CPUState *env)
 {
-    return !vm_running || env->stopped;
+    return !qemu_is_running() || env->stopped;
 }
 
 static void do_vm_stop(QemuState state)
 {
-    if (vm_running) {
+    if (qemu_is_running()) {
         cpu_disable_ticks();
-        vm_running = 0;
         pause_all_vcpus();
         qemu_state_set(state);
         vm_state_notify(0, state);
@@ -137,7 +136,7 @@ static int cpu_can_run(CPUState *env)
     if (env->stop) {
         return 0;
     }
-    if (env->stopped || !vm_running) {
+    if (env->stopped || !qemu_is_running()) {
         return 0;
     }
     return 1;
@@ -148,7 +147,7 @@ static bool cpu_thread_is_idle(CPUState *env)
     if (env->stop || env->queued_work_first) {
         return false;
     }
-    if (env->stopped || !vm_running) {
+    if (env->stopped || !qemu_is_running()) {
         return true;
     }
     if (!env->halted || qemu_cpu_has_work(env) ||
diff --git a/gdbstub.c b/gdbstub.c
index ae57d6a..63ba0a4 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2428,7 +2428,7 @@ static void gdb_read_byte(GDBState *s, int ch)
         if (ch != '$')
             return;
     }
-    if (vm_running) {
+    if (qemu_is_running()) {
         /* when the CPU is running, we cannot do anything except stop
            it when receiving a char */
         vm_stop(QSTATE_PAUSED);
@@ -2733,7 +2733,7 @@ static int gdb_monitor_write(CharDriverState *chr, const uint8_t *buf, int len)
 #ifndef _WIN32
 static void gdb_sigterm_handler(int signal)
 {
-    if (vm_running) {
+    if (qemu_is_running()) {
         vm_stop(QSTATE_PAUSED);
     }
 }
diff --git a/hw/etraxfs_dma.c b/hw/etraxfs_dma.c
index c205ec1..d9bf64a 100644
--- a/hw/etraxfs_dma.c
+++ b/hw/etraxfs_dma.c
@@ -732,7 +732,7 @@ static void DMA_run(void *opaque)
     struct fs_dma_ctrl *etraxfs_dmac = opaque;
     int p = 1;
 
-    if (vm_running)
+    if (qemu_is_running())
         p = etraxfs_dmac_run(etraxfs_dmac);
 
     if (p)
diff --git a/hw/kvmclock.c b/hw/kvmclock.c
index de5a655..ebf9da4 100644
--- a/hw/kvmclock.c
+++ b/hw/kvmclock.c
@@ -46,7 +46,7 @@ static void kvmclock_pre_save(void *opaque)
      * it on next vmsave (which would return a different value). Will be reset
      * when the VM is continued.
      */
-    s->clock_valid = !vm_running;
+    s->clock_valid = !qemu_is_running();
 }
 
 static int kvmclock_post_load(void *opaque, int version_id)
diff --git a/hw/virtio.c b/hw/virtio.c
index 7293db0..0d9f58a 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -869,7 +869,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
     vdev->queue_sel = 0;
     vdev->config_vector = VIRTIO_NO_VECTOR;
     vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
-    vdev->vm_running = vm_running;
+    vdev->vm_running = qemu_is_running();
     for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
         vdev->vq[i].vdev = vdev;
diff --git a/migration.c b/migration.c
index 73e9bd6..a073be8 100644
--- a/migration.c
+++ b/migration.c
@@ -376,7 +376,7 @@ void migrate_fd_put_ready(void *opaque)
     DPRINTF("iterate\n");
     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
         int state;
-        int old_vm_running = vm_running;
+        int old_vm_running = qemu_is_running();
 
         DPRINTF("done iterating\n");
         vm_stop(QSTATE_PREMIGRATE);
diff --git a/monitor.c b/monitor.c
index 05c5936..3fa2cf7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2618,7 +2618,7 @@ static void do_info_status_print(Monitor *mon, const QObject *data)
 static void do_info_status(Monitor *mon, QObject **ret_data)
 {
     *ret_data = qobject_from_jsonf("{ 'running': %i, 'singlestep': %i }",
-                                    vm_running, singlestep);
+                                    qemu_is_running(), singlestep);
 }
 
 static qemu_acl *find_acl(Monitor *mon, const char *name)
@@ -2811,7 +2811,7 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 static void do_loadvm(Monitor *mon, const QDict *qdict)
 {
-    int saved_vm_running  = vm_running;
+    int saved_vm_running  = qemu_is_running();
     const char *name = qdict_get_str(qdict, "name");
 
     vm_stop(QSTATE_RESTVM);
diff --git a/qemu-timer.c b/qemu-timer.c
index 5dbaaa3..319f197 100644
--- a/qemu-timer.c
+++ b/qemu-timer.c
@@ -246,7 +246,7 @@ static void icount_adjust(void)
     int64_t delta;
     static int64_t last_delta;
     /* If the VM is not running, then do nothing.  */
-    if (!vm_running)
+    if (!qemu_is_running())
         return;
 
     cur_time = cpu_get_clock();
@@ -404,7 +404,7 @@ static void icount_warp_rt(void *opaque)
         return;
     }
 
-    if (vm_running) {
+    if (qemu_is_running()) {
         int64_t clock = qemu_get_clock_ns(rt_clock);
         int64_t warp_delta = clock - vm_clock_warp_start;
         if (use_icount == 1) {
@@ -728,7 +728,7 @@ void qemu_run_all_timers(void)
     }
 
     /* vm time timers */
-    if (vm_running) {
+    if (qemu_is_running()) {
         qemu_run_timers(vm_clock);
     }
 
@@ -1182,7 +1182,7 @@ int qemu_calculate_timeout(void)
 #ifndef CONFIG_IOTHREAD
     int timeout;
 
-    if (!vm_running)
+    if (!qemu_is_running())
         timeout = 5000;
     else {
      /* XXX: use timeout computed from timers */
diff --git a/savevm.c b/savevm.c
index d8db493..fbfcdfa 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1602,7 +1602,7 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f)
     int saved_vm_running;
     int ret;
 
-    saved_vm_running = vm_running;
+    saved_vm_running = qemu_is_running();
     vm_stop(QSTATE_SAVEVM);
 
     if (qemu_savevm_state_blocked(mon)) {
@@ -1931,7 +1931,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    saved_vm_running = vm_running;
+    saved_vm_running = qemu_is_running();
     vm_stop(QSTATE_SAVEVM);
 
     memset(sn, 0, sizeof(*sn));
diff --git a/sysemu.h b/sysemu.h
index 32c9abb..46079ab 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -31,12 +31,12 @@ typedef enum {
 
 extern const char *bios_name;
 
-extern int vm_running;
 extern const char *qemu_name;
 extern uint8_t qemu_uuid[];
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
+int qemu_is_running(void);
 QemuState qemu_state_get(void);
 void qemu_state_set(QemuState state);
 typedef struct vm_change_state_entry VMChangeStateEntry;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index c88cd14..531589c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1115,7 +1115,7 @@ static int kvm_get_msrs(CPUState *env)
 
     if (!env->tsc_valid) {
         msrs[n++].index = MSR_IA32_TSC;
-        env->tsc_valid = !vm_running;
+        env->tsc_valid = !qemu_is_running();
     }
 
 #ifdef TARGET_X86_64
diff --git a/ui/sdl.c b/ui/sdl.c
index 6dbc5cb..ba2363c 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -407,7 +407,7 @@ static void sdl_update_caption(void)
     char icon_title[1024];
     const char *status = "";
 
-    if (!vm_running)
+    if (!qemu_is_running())
         status = " [Stopped]";
     else if (gui_grab) {
         if (alt_grab)
@@ -545,8 +545,8 @@ static void sdl_refresh(DisplayState *ds)
     int mod_state;
     int buttonstate = SDL_GetMouseState(NULL, NULL);
 
-    if (last_vm_running != vm_running) {
-        last_vm_running = vm_running;
+    if (last_vm_running != qemu_is_running()) {
+        last_vm_running = qemu_is_running();
         sdl_update_caption();
     }
 
diff --git a/vl.c b/vl.c
index c2ab7ce..65cf4a5 100644
--- a/vl.c
+++ b/vl.c
@@ -183,7 +183,6 @@ int mem_prealloc = 0; /* force preallocation of physical target memory */
 #endif
 int nb_nics;
 NICInfo nd_table[MAX_NICS];
-int vm_running;
 int autostart;
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
@@ -334,6 +333,11 @@ void qemu_state_set(QemuState state)
     qemu_current_state = state;
 }
 
+int qemu_is_running(void)
+{
+    return qemu_current_state == QSTATE_RUNNING;
+}
+
 /***********************************************************/
 /* real time host monotonic timer */
 
@@ -1170,9 +1174,8 @@ void vm_state_notify(int running, QemuState state)
 
 void vm_start(void)
 {
-    if (!vm_running) {
+    if (!qemu_is_running()) {
         cpu_enable_ticks();
-        vm_running = 1;
         qemu_state_set(QSTATE_RUNNING);
         vm_state_notify(1, QSTATE_RUNNING);
         resume_all_vcpus();
diff --git a/xen-all.c b/xen-all.c
index 5d2bc4c..fbbdf77 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -736,7 +736,7 @@ static void cpu_handle_ioreq(void *opaque)
          * guest resumes and does a hlt with interrupts disabled which
          * causes Xen to powerdown the domain.
          */
-        if (vm_running) {
+        if (qemu_is_running()) {
             if (qemu_shutdown_requested_get()) {
                 destroy_hvm_domain();
             }
-- 
1.7.6.396.ge0613

[Qemu-devel] [PATCH 6/7] Monitor: Don't allow cont on bad VM state

[Luiz Capitulino]

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;
+        }
     }
+
     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 */
-- 
1.7.6.396.ge0613

Re: [Qemu-devel] [PATCH 6/7] Monitor: Don't allow cont on bad VM state

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 6/7] Monitor: Don't allow cont on bad VM state

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH 6/7] Monitor: Don't allow cont on bad VM state

[Jan Kiszka]

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 7/7] QMP: query-status: Introduce 'status' key

[Luiz Capitulino]

This new key reports the current VM status to clients. Please, check
the documentation being added in this commit for more details.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    3 +--
 qmp-commands.hx |   21 ++++++++++++++++++++-
 sysemu.h        |    1 +
 vl.c            |   24 ++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index f1cb5af..a444924 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2625,8 +2625,7 @@ static void do_info_status_print(Monitor *mon, const QObject *data)
 
 static void do_info_status(Monitor *mon, QObject **ret_data)
 {
-    *ret_data = qobject_from_jsonf("{ 'running': %i, 'singlestep': %i }",
-                                    qemu_is_running(), singlestep);
+    *ret_data = qobject_from_jsonf("{ 'running': %i, 'singlestep': %i, 'status': %s }", qemu_is_running(), singlestep, qemu_state_get_name());
 }
 
 static qemu_acl *find_acl(Monitor *mon, const char *name)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 03f67da..87e042f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1547,11 +1547,30 @@ Return a json-object with the following information:
 - "running": true if the VM is running, or false if it is paused (json-bool)
 - "singlestep": true if the VM is in single step mode,
                 false otherwise (json-bool)
+- "status": one of the following values (json-string)
+    "debug" - QEMU is running on a debugger
+    "inmigrate" - guest is paused waiting for an incoming migration
+    "internal-error" - An internal error that prevents further guest
+    execution has occurred
+    "io-error" - the last IOP has failed and the device is configured 
+    to pause on I/O errors
+    "paused" - guest has been paused via the 'stop' command
+    "postmigrate" - guest is paused following a successful 'migrate'
+    "prelaunch" - QEMU was started with -S and guest has not started
+    "finish-migrate" - guest is paused to finish the migration process
+    "restore-vm" - guest is paused to restore VM state
+    "restore-vm-failed" - guest is paused following a failed attempt to
+    restore the VM state
+    "running" - guest is actively running
+    "save-vm" - guest is paused to save the VM state
+    "shutdown" - guest is shut down (and -no-shutdown is in use)
+    "watchdog" - the watchdog action is configured to pause and 
+     has been triggered
 
 Example:
 
 -> { "execute": "query-status" }
-<- { "return": { "running": true, "singlestep": false } }
+<- { "return": { "running": true, "singlestep": false, "status": "running" } }
 
 EQMP
 
diff --git a/sysemu.h b/sysemu.h
index 12a3f6a..6ae4db9 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -38,6 +38,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
 
 int qemu_is_running(void);
 QemuState qemu_state_get(void);
+const char *qemu_state_get_name(void);
 void qemu_state_set(QemuState state);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, QemuState state);
diff --git a/vl.c b/vl.c
index 7fad355..e41c32f 100644
--- a/vl.c
+++ b/vl.c
@@ -321,6 +321,23 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
 /***********************************************************/
 /* QEMU state */
 
+static const char *const qemu_state_name_tbl[QSTATE_MAX] = {
+    [QSTATE_DEBUG] = "debug",
+    [QSTATE_INMIGRATE] = "incoming-migration",
+    [QSTATE_INTERROR] = "internal-error",
+    [QSTATE_IOERROR] = "io-error",
+    [QSTATE_PAUSED] = "paused",
+    [QSTATE_POSTMIGRATE] = "post-migrate",
+    [QSTATE_PRELAUNCH] = "prelaunch",
+    [QSTATE_PREMIGRATE] = "finish-migrate",
+    [QSTATE_RESTVM] = "restore-vm",
+    [QSTATE_RESTVMFAILED] = "restore-vm-failed",
+    [QSTATE_RUNNING] = "running",
+    [QSTATE_SAVEVM] = "save-vm",
+    [QSTATE_SHUTDOWN] = "shutdown",
+    [QSTATE_WATCHDOG] = "watchdog",
+};
+
 static QemuState qemu_current_state = QSTATE_NOSTATE;
 
 QemuState qemu_state_get(void)
@@ -334,6 +351,13 @@ void qemu_state_set(QemuState state)
     qemu_current_state = state;
 }
 
+const char *qemu_state_get_name(void)
+{
+    assert(qemu_current_state > QSTATE_NOSTATE &&
+           qemu_current_state < QSTATE_MAX);
+    return qemu_state_name_tbl[qemu_current_state];
+}
+
 int qemu_is_running(void)
 {
     return qemu_current_state == QSTATE_RUNNING;
-- 
1.7.6.396.ge0613

Re: [Qemu-devel] [PATCH 0/7]: Introduce the QemuState type

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> It replaces the VMSTOP macros and allows us to drop some global variables.
>
> Also, the problem with issuing 'cont' when the VM is in bad state is addressed,
> and we make the current state available in QMP.

Looks pretty good to me.