* [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-05 18:33 ` Anthony Liguori
2011-07-12 7:28 ` Markus Armbruster
2011-07-05 18:17 ` [Qemu-devel] [PATCH 2/8] QMP: query-status: Introduce 'status' key Luiz Capitulino
` (7 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
We need to track the VM status so that QMP can report it to clients.
This commit adds the VMStatus type and related functions. The
vm_status_set() function is used to keep track of the current
VM status.
The current statuses are:
- debug: guest is running under gdb
- inmigrate: guest is paused waiting for an incoming migration
- postmigrate: guest is paused following a successful migration
- internal-error: Fatal internal error that prevents further guest
execution
- load-state-error: guest is paused following a failed 'loadvm'
- io-error: the last IOP has failed and the device is configured
to pause on I/O errors
- watchdog-error: the watchdog action is configured to pause and
has been triggered
- paused: guest has been paused via the 'stop' command
- prelaunch: QEMU was started with -S and guest has not started
- running: guest is actively running
- shutdown: guest is shut down (and -no-shutdown is in use)
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
gdbstub.c | 4 ++++
hw/ide/core.c | 1 +
hw/scsi-disk.c | 1 +
hw/virtio-blk.c | 1 +
hw/watchdog.c | 1 +
kvm-all.c | 1 +
migration.c | 3 +++
monitor.c | 5 ++++-
sysemu.h | 19 +++++++++++++++++++
vl.c | 37 +++++++++++++++++++++++++++++++++++++
10 files changed, 72 insertions(+), 1 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index c085a5a..61b700a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
s->state = RS_SYSCALL;
#ifndef CONFIG_USER_ONLY
vm_stop(VMSTOP_DEBUG);
+ vm_status_set(VMST_DEBUG);
#endif
s->state = RS_IDLE;
va_start(va, fmt);
@@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
/* when the CPU is running, we cannot do anything except stop
it when receiving a char */
vm_stop(VMSTOP_USER);
+ vm_status_set(VMST_DEBUG);
} else
#endif
{
@@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_OPENED:
vm_stop(VMSTOP_USER);
+ vm_status_set(VMST_DEBUG);
gdb_has_xml = 0;
break;
default:
@@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
{
if (vm_running) {
vm_stop(VMSTOP_USER);
+ vm_status_set(VMST_DEBUG);
}
}
#endif
diff --git a/hw/ide/core.c b/hw/ide/core.c
index ca17a43..bf9df41 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
s->bus->error_status = op;
bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
vm_stop(VMSTOP_DISKFULL);
+ vm_status_set(VMST_IOERROR);
} else {
if (op & BM_STATUS_DMA_RETRY) {
dma_buf_commit(s, 0);
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a8c7372..66037fd 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
vm_stop(VMSTOP_DISKFULL);
+ vm_status_set(VMST_IOERROR);
} else {
if (type == SCSI_REQ_STATUS_RETRY_READ) {
scsi_req_data(&r->req, 0);
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 91e0394..bf70200 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
s->rq = req;
bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
vm_stop(VMSTOP_DISKFULL);
+ vm_status_set(VMST_IOERROR);
} else {
virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
diff --git a/hw/watchdog.c b/hw/watchdog.c
index 1c900a1..d130cbb 100644
--- a/hw/watchdog.c
+++ b/hw/watchdog.c
@@ -133,6 +133,7 @@ void watchdog_perform_action(void)
case WDT_PAUSE: /* same as 'stop' command in monitor */
watchdog_mon_event("pause");
vm_stop(VMSTOP_WATCHDOG);
+ vm_status_set(VMST_WATCHDOG);
break;
case WDT_DEBUG:
diff --git a/kvm-all.c b/kvm-all.c
index cbc2532..aee9e0a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
if (ret < 0) {
cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
vm_stop(VMSTOP_PANIC);
+ vm_status_set(VMST_INTERROR);
}
env->exit_request = 0;
diff --git a/migration.c b/migration.c
index af3a1f2..674792f 100644
--- a/migration.c
+++ b/migration.c
@@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
}
state = MIG_STATE_ERROR;
}
+ if (state == MIG_STATE_COMPLETED) {
+ vm_status_set(VMST_POSTMIGRATE);
+ }
s->state = state;
notifier_list_notify(&migration_state_notifiers);
}
diff --git a/monitor.c b/monitor.c
index 67ceb46..1cb3191 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1258,6 +1258,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_status_set(VMST_PAUSED);
return 0;
}
@@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
vm_stop(VMSTOP_LOADVM);
- if (load_vmstate(name) == 0 && saved_vm_running) {
+ if (load_vmstate(name) < 0) {
+ vm_status_set(VMST_LOADERROR);
+ } else if (saved_vm_running) {
vm_start();
}
}
diff --git a/sysemu.h b/sysemu.h
index d3013f5..7308ac5 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
int qemu_loadvm_state(QEMUFile *f);
+typedef enum {
+ VMST_NOSTATUS,
+ VMST_DEBUG,
+ VMST_INMIGRATE,
+ VMST_POSTMIGRATE,
+ VMST_INTERROR,
+ VMST_IOERROR,
+ VMST_LOADERROR,
+ VMST_PAUSED,
+ VMST_PRELAUNCH,
+ VMST_RUNNING,
+ VMST_SHUTDOWN,
+ VMST_WATCHDOG,
+ VMST_MAX,
+} VMStatus;
+
+void vm_status_set(VMStatus s);
+const char *vm_status_get_name(void);
+
/* SLIRP */
void do_info_slirp(Monitor *mon);
diff --git a/vl.c b/vl.c
index fcd7395..fb7208f 100644
--- a/vl.c
+++ b/vl.c
@@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
}
/***********************************************************/
+/* VMStatus */
+
+static const char *const vm_status_name[VMST_MAX] = {
+ [VMST_DEBUG] = "debug",
+ [VMST_INMIGRATE] = "inmigrate",
+ [VMST_POSTMIGRATE] = "postmigrate",
+ [VMST_INTERROR] = "internal-error",
+ [VMST_IOERROR] = "io-error",
+ [VMST_LOADERROR] = "load-state-error",
+ [VMST_PAUSED] = "paused",
+ [VMST_PRELAUNCH] = "prelaunch",
+ [VMST_RUNNING] = "running",
+ [VMST_SHUTDOWN] = "shutdown",
+ [VMST_WATCHDOG] = "watchdog-error",
+};
+
+static VMStatus qemu_vm_status = VMST_NOSTATUS;
+
+void vm_status_set(VMStatus s)
+{
+ assert(s > VMST_NOSTATUS && s < VMST_MAX);
+ qemu_vm_status = s;
+}
+
+const char *vm_status_get_name(void)
+{
+ assert(qemu_vm_status > VMST_NOSTATUS && qemu_vm_status < VMST_MAX);
+ return vm_status_name[qemu_vm_status];
+}
+
+/***********************************************************/
/* real time host monotonic timer */
/***********************************************************/
@@ -1150,6 +1181,7 @@ void vm_start(void)
vm_state_notify(1, 0);
resume_all_vcpus();
monitor_protocol_event(QEVENT_RESUME, NULL);
+ vm_status_set(VMST_RUNNING);
}
}
@@ -1392,12 +1424,14 @@ static void main_loop(void)
if (qemu_debug_requested()) {
vm_stop(VMSTOP_DEBUG);
+ vm_status_set(VMST_DEBUG);
}
if (qemu_shutdown_requested()) {
qemu_kill_report();
monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
if (no_shutdown) {
vm_stop(VMSTOP_SHUTDOWN);
+ vm_status_set(VMST_SHUTDOWN);
no_shutdown = 0;
} else
break;
@@ -2476,6 +2510,7 @@ int main(int argc, char **argv, char **envp)
break;
case QEMU_OPTION_S:
autostart = 0;
+ vm_status_set(VMST_PRELAUNCH);
break;
case QEMU_OPTION_k:
keyboard_layout = optarg;
@@ -3307,11 +3342,13 @@ int main(int argc, char **argv, char **envp)
qemu_system_reset(VMRESET_SILENT);
if (loadvm) {
if (load_vmstate(loadvm) < 0) {
+ vm_status_set(VMST_LOADERROR);
autostart = 0;
}
}
if (incoming) {
+ vm_status_set(VMST_INMIGRATE);
int ret = qemu_start_incoming_migration(incoming);
if (ret < 0) {
fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-05 18:17 ` [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type Luiz Capitulino
@ 2011-07-05 18:33 ` Anthony Liguori
2011-07-05 18:51 ` Luiz Capitulino
2011-07-12 7:28 ` Markus Armbruster
1 sibling, 1 reply; 29+ messages in thread
From: Anthony Liguori @ 2011-07-05 18:33 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, jan.kiszka, qemu-devel, armbru, stefanha, jdenemar
On 07/05/2011 01:17 PM, Luiz Capitulino wrote:
> We need to track the VM status so that QMP can report it to clients.
>
> This commit adds the VMStatus type and related functions. The
> vm_status_set() function is used to keep track of the current
> VM status.
>
> The current statuses are:
Which states are terminal, and what's the proper procedure to recover
from non-terminal states? Is it cont or system_reset followed by cont?
>
> - debug: guest is running under gdb
> - inmigrate: guest is paused waiting for an incoming migration
> - postmigrate: guest is paused following a successful migration
> - internal-error: Fatal internal error that prevents further guest
> execution
> - load-state-error: guest is paused following a failed 'loadvm'
> - io-error: the last IOP has failed and the device is configured
> to pause on I/O errors
> - watchdog-error: the watchdog action is configured to pause and
> has been triggered
> - paused: guest has been paused via the 'stop' command
> - prelaunch: QEMU was started with -S and guest has not started
> - running: guest is actively running
> - shutdown: guest is shut down (and -no-shutdown is in use)
Regards,
Anthony Liguori
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> ---
> gdbstub.c | 4 ++++
> hw/ide/core.c | 1 +
> hw/scsi-disk.c | 1 +
> hw/virtio-blk.c | 1 +
> hw/watchdog.c | 1 +
> kvm-all.c | 1 +
> migration.c | 3 +++
> monitor.c | 5 ++++-
> sysemu.h | 19 +++++++++++++++++++
> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> 10 files changed, 72 insertions(+), 1 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index c085a5a..61b700a 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> s->state = RS_SYSCALL;
> #ifndef CONFIG_USER_ONLY
> vm_stop(VMSTOP_DEBUG);
> + vm_status_set(VMST_DEBUG);
> #endif
> s->state = RS_IDLE;
> va_start(va, fmt);
> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> /* when the CPU is running, we cannot do anything except stop
> it when receiving a char */
> vm_stop(VMSTOP_USER);
> + vm_status_set(VMST_DEBUG);
> } else
> #endif
> {
> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> switch (event) {
> case CHR_EVENT_OPENED:
> vm_stop(VMSTOP_USER);
> + vm_status_set(VMST_DEBUG);
> gdb_has_xml = 0;
> break;
> default:
> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> {
> if (vm_running) {
> vm_stop(VMSTOP_USER);
> + vm_status_set(VMST_DEBUG);
> }
> }
> #endif
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index ca17a43..bf9df41 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> s->bus->error_status = op;
> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> vm_stop(VMSTOP_DISKFULL);
> + vm_status_set(VMST_IOERROR);
> } else {
> if (op& BM_STATUS_DMA_RETRY) {
> dma_buf_commit(s, 0);
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index a8c7372..66037fd 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>
> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> vm_stop(VMSTOP_DISKFULL);
> + vm_status_set(VMST_IOERROR);
> } else {
> if (type == SCSI_REQ_STATUS_RETRY_READ) {
> scsi_req_data(&r->req, 0);
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 91e0394..bf70200 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> s->rq = req;
> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> vm_stop(VMSTOP_DISKFULL);
> + vm_status_set(VMST_IOERROR);
> } else {
> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> diff --git a/hw/watchdog.c b/hw/watchdog.c
> index 1c900a1..d130cbb 100644
> --- a/hw/watchdog.c
> +++ b/hw/watchdog.c
> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> case WDT_PAUSE: /* same as 'stop' command in monitor */
> watchdog_mon_event("pause");
> vm_stop(VMSTOP_WATCHDOG);
> + vm_status_set(VMST_WATCHDOG);
> break;
>
> case WDT_DEBUG:
> diff --git a/kvm-all.c b/kvm-all.c
> index cbc2532..aee9e0a 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> if (ret< 0) {
> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> vm_stop(VMSTOP_PANIC);
> + vm_status_set(VMST_INTERROR);
> }
>
> env->exit_request = 0;
> diff --git a/migration.c b/migration.c
> index af3a1f2..674792f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> }
> state = MIG_STATE_ERROR;
> }
> + if (state == MIG_STATE_COMPLETED) {
> + vm_status_set(VMST_POSTMIGRATE);
> + }
> s->state = state;
> notifier_list_notify(&migration_state_notifiers);
> }
> diff --git a/monitor.c b/monitor.c
> index 67ceb46..1cb3191 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> return 0;
> }
>
> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>
> vm_stop(VMSTOP_LOADVM);
>
> - if (load_vmstate(name) == 0&& saved_vm_running) {
> + if (load_vmstate(name)< 0) {
> + vm_status_set(VMST_LOADERROR);
> + } else if (saved_vm_running) {
> vm_start();
> }
> }
> diff --git a/sysemu.h b/sysemu.h
> index d3013f5..7308ac5 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> int qemu_loadvm_state(QEMUFile *f);
>
> +typedef enum {
> + VMST_NOSTATUS,
> + VMST_DEBUG,
> + VMST_INMIGRATE,
> + VMST_POSTMIGRATE,
> + VMST_INTERROR,
> + VMST_IOERROR,
> + VMST_LOADERROR,
> + VMST_PAUSED,
> + VMST_PRELAUNCH,
> + VMST_RUNNING,
> + VMST_SHUTDOWN,
> + VMST_WATCHDOG,
> + VMST_MAX,
> +} VMStatus;
> +
> +void vm_status_set(VMStatus s);
> +const char *vm_status_get_name(void);
> +
> /* SLIRP */
> void do_info_slirp(Monitor *mon);
>
> diff --git a/vl.c b/vl.c
> index fcd7395..fb7208f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> }
>
> /***********************************************************/
> +/* VMStatus */
> +
> +static const char *const vm_status_name[VMST_MAX] = {
> + [VMST_DEBUG] = "debug",
> + [VMST_INMIGRATE] = "inmigrate",
> + [VMST_POSTMIGRATE] = "postmigrate",
> + [VMST_INTERROR] = "internal-error",
> + [VMST_IOERROR] = "io-error",
> + [VMST_LOADERROR] = "load-state-error",
> + [VMST_PAUSED] = "paused",
> + [VMST_PRELAUNCH] = "prelaunch",
> + [VMST_RUNNING] = "running",
> + [VMST_SHUTDOWN] = "shutdown",
> + [VMST_WATCHDOG] = "watchdog-error",
> +};
> +
> +static VMStatus qemu_vm_status = VMST_NOSTATUS;
> +
> +void vm_status_set(VMStatus s)
> +{
> + assert(s> VMST_NOSTATUS&& s< VMST_MAX);
> + qemu_vm_status = s;
> +}
> +
> +const char *vm_status_get_name(void)
> +{
> + assert(qemu_vm_status> VMST_NOSTATUS&& qemu_vm_status< VMST_MAX);
> + return vm_status_name[qemu_vm_status];
> +}
> +
> +/***********************************************************/
> /* real time host monotonic timer */
>
> /***********************************************************/
> @@ -1150,6 +1181,7 @@ void vm_start(void)
> vm_state_notify(1, 0);
> resume_all_vcpus();
> monitor_protocol_event(QEVENT_RESUME, NULL);
> + vm_status_set(VMST_RUNNING);
> }
> }
>
> @@ -1392,12 +1424,14 @@ static void main_loop(void)
>
> if (qemu_debug_requested()) {
> vm_stop(VMSTOP_DEBUG);
> + vm_status_set(VMST_DEBUG);
> }
> if (qemu_shutdown_requested()) {
> qemu_kill_report();
> monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
> if (no_shutdown) {
> vm_stop(VMSTOP_SHUTDOWN);
> + vm_status_set(VMST_SHUTDOWN);
> no_shutdown = 0;
> } else
> break;
> @@ -2476,6 +2510,7 @@ int main(int argc, char **argv, char **envp)
> break;
> case QEMU_OPTION_S:
> autostart = 0;
> + vm_status_set(VMST_PRELAUNCH);
> break;
> case QEMU_OPTION_k:
> keyboard_layout = optarg;
> @@ -3307,11 +3342,13 @@ int main(int argc, char **argv, char **envp)
> qemu_system_reset(VMRESET_SILENT);
> if (loadvm) {
> if (load_vmstate(loadvm)< 0) {
> + vm_status_set(VMST_LOADERROR);
> autostart = 0;
> }
> }
>
> if (incoming) {
> + vm_status_set(VMST_INMIGRATE);
> int ret = qemu_start_incoming_migration(incoming);
> if (ret< 0) {
> fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-05 18:33 ` Anthony Liguori
@ 2011-07-05 18:51 ` Luiz Capitulino
2011-07-05 18:58 ` Anthony Liguori
0 siblings, 1 reply; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:51 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kwolf, jan.kiszka, qemu-devel, armbru, stefanha, jdenemar
On Tue, 05 Jul 2011 13:33:07 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 07/05/2011 01:17 PM, Luiz Capitulino wrote:
> > We need to track the VM status so that QMP can report it to clients.
> >
> > This commit adds the VMStatus type and related functions. The
> > vm_status_set() function is used to keep track of the current
> > VM status.
> >
> > The current statuses are:
>
> Which states are terminal, and what's the proper procedure to recover
> from non-terminal states? Is it cont or system_reset followed by cont?
Can I add that to the next patch (which also adds the QMP doc) or do you
prefer it in this patch?
>
> >
> > - debug: guest is running under gdb
> > - inmigrate: guest is paused waiting for an incoming migration
> > - postmigrate: guest is paused following a successful migration
> > - internal-error: Fatal internal error that prevents further guest
> > execution
> > - load-state-error: guest is paused following a failed 'loadvm'
> > - io-error: the last IOP has failed and the device is configured
> > to pause on I/O errors
> > - watchdog-error: the watchdog action is configured to pause and
> > has been triggered
> > - paused: guest has been paused via the 'stop' command
> > - prelaunch: QEMU was started with -S and guest has not started
> > - running: guest is actively running
> > - shutdown: guest is shut down (and -no-shutdown is in use)
>
> Regards,
>
> Anthony Liguori
>
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> > ---
> > gdbstub.c | 4 ++++
> > hw/ide/core.c | 1 +
> > hw/scsi-disk.c | 1 +
> > hw/virtio-blk.c | 1 +
> > hw/watchdog.c | 1 +
> > kvm-all.c | 1 +
> > migration.c | 3 +++
> > monitor.c | 5 ++++-
> > sysemu.h | 19 +++++++++++++++++++
> > vl.c | 37 +++++++++++++++++++++++++++++++++++++
> > 10 files changed, 72 insertions(+), 1 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index c085a5a..61b700a 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> > s->state = RS_SYSCALL;
> > #ifndef CONFIG_USER_ONLY
> > vm_stop(VMSTOP_DEBUG);
> > + vm_status_set(VMST_DEBUG);
> > #endif
> > s->state = RS_IDLE;
> > va_start(va, fmt);
> > @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> > /* when the CPU is running, we cannot do anything except stop
> > it when receiving a char */
> > vm_stop(VMSTOP_USER);
> > + vm_status_set(VMST_DEBUG);
> > } else
> > #endif
> > {
> > @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> > switch (event) {
> > case CHR_EVENT_OPENED:
> > vm_stop(VMSTOP_USER);
> > + vm_status_set(VMST_DEBUG);
> > gdb_has_xml = 0;
> > break;
> > default:
> > @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> > {
> > if (vm_running) {
> > vm_stop(VMSTOP_USER);
> > + vm_status_set(VMST_DEBUG);
> > }
> > }
> > #endif
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index ca17a43..bf9df41 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> > s->bus->error_status = op;
> > bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > vm_stop(VMSTOP_DISKFULL);
> > + vm_status_set(VMST_IOERROR);
> > } else {
> > if (op& BM_STATUS_DMA_RETRY) {
> > dma_buf_commit(s, 0);
> > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> > index a8c7372..66037fd 100644
> > --- a/hw/scsi-disk.c
> > +++ b/hw/scsi-disk.c
> > @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> >
> > bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > vm_stop(VMSTOP_DISKFULL);
> > + vm_status_set(VMST_IOERROR);
> > } else {
> > if (type == SCSI_REQ_STATUS_RETRY_READ) {
> > scsi_req_data(&r->req, 0);
> > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > index 91e0394..bf70200 100644
> > --- a/hw/virtio-blk.c
> > +++ b/hw/virtio-blk.c
> > @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> > s->rq = req;
> > bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > vm_stop(VMSTOP_DISKFULL);
> > + vm_status_set(VMST_IOERROR);
> > } else {
> > virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> > bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> > diff --git a/hw/watchdog.c b/hw/watchdog.c
> > index 1c900a1..d130cbb 100644
> > --- a/hw/watchdog.c
> > +++ b/hw/watchdog.c
> > @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> > case WDT_PAUSE: /* same as 'stop' command in monitor */
> > watchdog_mon_event("pause");
> > vm_stop(VMSTOP_WATCHDOG);
> > + vm_status_set(VMST_WATCHDOG);
> > break;
> >
> > case WDT_DEBUG:
> > diff --git a/kvm-all.c b/kvm-all.c
> > index cbc2532..aee9e0a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> > if (ret< 0) {
> > cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> > vm_stop(VMSTOP_PANIC);
> > + vm_status_set(VMST_INTERROR);
> > }
> >
> > env->exit_request = 0;
> > diff --git a/migration.c b/migration.c
> > index af3a1f2..674792f 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> > }
> > state = MIG_STATE_ERROR;
> > }
> > + if (state == MIG_STATE_COMPLETED) {
> > + vm_status_set(VMST_POSTMIGRATE);
> > + }
> > s->state = state;
> > notifier_list_notify(&migration_state_notifiers);
> > }
> > diff --git a/monitor.c b/monitor.c
> > index 67ceb46..1cb3191 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> > return 0;
> > }
> >
> > @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
> >
> > vm_stop(VMSTOP_LOADVM);
> >
> > - if (load_vmstate(name) == 0&& saved_vm_running) {
> > + if (load_vmstate(name)< 0) {
> > + vm_status_set(VMST_LOADERROR);
> > + } else if (saved_vm_running) {
> > vm_start();
> > }
> > }
> > diff --git a/sysemu.h b/sysemu.h
> > index d3013f5..7308ac5 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> > void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> > int qemu_loadvm_state(QEMUFile *f);
> >
> > +typedef enum {
> > + VMST_NOSTATUS,
> > + VMST_DEBUG,
> > + VMST_INMIGRATE,
> > + VMST_POSTMIGRATE,
> > + VMST_INTERROR,
> > + VMST_IOERROR,
> > + VMST_LOADERROR,
> > + VMST_PAUSED,
> > + VMST_PRELAUNCH,
> > + VMST_RUNNING,
> > + VMST_SHUTDOWN,
> > + VMST_WATCHDOG,
> > + VMST_MAX,
> > +} VMStatus;
> > +
> > +void vm_status_set(VMStatus s);
> > +const char *vm_status_get_name(void);
> > +
> > /* SLIRP */
> > void do_info_slirp(Monitor *mon);
> >
> > diff --git a/vl.c b/vl.c
> > index fcd7395..fb7208f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> > }
> >
> > /***********************************************************/
> > +/* VMStatus */
> > +
> > +static const char *const vm_status_name[VMST_MAX] = {
> > + [VMST_DEBUG] = "debug",
> > + [VMST_INMIGRATE] = "inmigrate",
> > + [VMST_POSTMIGRATE] = "postmigrate",
> > + [VMST_INTERROR] = "internal-error",
> > + [VMST_IOERROR] = "io-error",
> > + [VMST_LOADERROR] = "load-state-error",
> > + [VMST_PAUSED] = "paused",
> > + [VMST_PRELAUNCH] = "prelaunch",
> > + [VMST_RUNNING] = "running",
> > + [VMST_SHUTDOWN] = "shutdown",
> > + [VMST_WATCHDOG] = "watchdog-error",
> > +};
> > +
> > +static VMStatus qemu_vm_status = VMST_NOSTATUS;
> > +
> > +void vm_status_set(VMStatus s)
> > +{
> > + assert(s> VMST_NOSTATUS&& s< VMST_MAX);
> > + qemu_vm_status = s;
> > +}
> > +
> > +const char *vm_status_get_name(void)
> > +{
> > + assert(qemu_vm_status> VMST_NOSTATUS&& qemu_vm_status< VMST_MAX);
> > + return vm_status_name[qemu_vm_status];
> > +}
> > +
> > +/***********************************************************/
> > /* real time host monotonic timer */
> >
> > /***********************************************************/
> > @@ -1150,6 +1181,7 @@ void vm_start(void)
> > vm_state_notify(1, 0);
> > resume_all_vcpus();
> > monitor_protocol_event(QEVENT_RESUME, NULL);
> > + vm_status_set(VMST_RUNNING);
> > }
> > }
> >
> > @@ -1392,12 +1424,14 @@ static void main_loop(void)
> >
> > if (qemu_debug_requested()) {
> > vm_stop(VMSTOP_DEBUG);
> > + vm_status_set(VMST_DEBUG);
> > }
> > if (qemu_shutdown_requested()) {
> > qemu_kill_report();
> > monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
> > if (no_shutdown) {
> > vm_stop(VMSTOP_SHUTDOWN);
> > + vm_status_set(VMST_SHUTDOWN);
> > no_shutdown = 0;
> > } else
> > break;
> > @@ -2476,6 +2510,7 @@ int main(int argc, char **argv, char **envp)
> > break;
> > case QEMU_OPTION_S:
> > autostart = 0;
> > + vm_status_set(VMST_PRELAUNCH);
> > break;
> > case QEMU_OPTION_k:
> > keyboard_layout = optarg;
> > @@ -3307,11 +3342,13 @@ int main(int argc, char **argv, char **envp)
> > qemu_system_reset(VMRESET_SILENT);
> > if (loadvm) {
> > if (load_vmstate(loadvm)< 0) {
> > + vm_status_set(VMST_LOADERROR);
> > autostart = 0;
> > }
> > }
> >
> > if (incoming) {
> > + vm_status_set(VMST_INMIGRATE);
> > int ret = qemu_start_incoming_migration(incoming);
> > if (ret< 0) {
> > fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-05 18:51 ` Luiz Capitulino
@ 2011-07-05 18:58 ` Anthony Liguori
2011-07-05 19:34 ` Luiz Capitulino
0 siblings, 1 reply; 29+ messages in thread
From: Anthony Liguori @ 2011-07-05 18:58 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, jan.kiszka, qemu-devel, armbru, stefanha, jdenemar
On 07/05/2011 01:51 PM, Luiz Capitulino wrote:
> On Tue, 05 Jul 2011 13:33:07 -0500
> Anthony Liguori<anthony@codemonkey.ws> wrote:
>
>> On 07/05/2011 01:17 PM, Luiz Capitulino wrote:
>>> We need to track the VM status so that QMP can report it to clients.
>>>
>>> This commit adds the VMStatus type and related functions. The
>>> vm_status_set() function is used to keep track of the current
>>> VM status.
>>>
>>> The current statuses are:
>>
>> Which states are terminal, and what's the proper procedure to recover
>> from non-terminal states? Is it cont or system_reset followed by cont?
>
> Can I add that to the next patch (which also adds the QMP doc) or do you
> prefer it in this patch?
It can be a separate patch, I was more curious about whether you had
thought through this for each of the states so that the states we were
introducing were well defined.
For instance, it's not clear to me what the semantics of
'load-state-error' are and how it's different from prelaunch.
I'm quite surprised that 'load-state-error' wouldn't be a terminal error.
Regards,
Anthony Liguori
>
>>
>>>
>>> - debug: guest is running under gdb
>>> - inmigrate: guest is paused waiting for an incoming migration
>>> - postmigrate: guest is paused following a successful migration
>>> - internal-error: Fatal internal error that prevents further guest
>>> execution
>>> - load-state-error: guest is paused following a failed 'loadvm'
>>> - io-error: the last IOP has failed and the device is configured
>>> to pause on I/O errors
>>> - watchdog-error: the watchdog action is configured to pause and
>>> has been triggered
>>> - paused: guest has been paused via the 'stop' command
>>> - prelaunch: QEMU was started with -S and guest has not started
>>> - running: guest is actively running
>>> - shutdown: guest is shut down (and -no-shutdown is in use)
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>> gdbstub.c | 4 ++++
>>> hw/ide/core.c | 1 +
>>> hw/scsi-disk.c | 1 +
>>> hw/virtio-blk.c | 1 +
>>> hw/watchdog.c | 1 +
>>> kvm-all.c | 1 +
>>> migration.c | 3 +++
>>> monitor.c | 5 ++++-
>>> sysemu.h | 19 +++++++++++++++++++
>>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
>>> 10 files changed, 72 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index c085a5a..61b700a 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
>>> s->state = RS_SYSCALL;
>>> #ifndef CONFIG_USER_ONLY
>>> vm_stop(VMSTOP_DEBUG);
>>> + vm_status_set(VMST_DEBUG);
>>> #endif
>>> s->state = RS_IDLE;
>>> va_start(va, fmt);
>>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>>> /* when the CPU is running, we cannot do anything except stop
>>> it when receiving a char */
>>> vm_stop(VMSTOP_USER);
>>> + vm_status_set(VMST_DEBUG);
>>> } else
>>> #endif
>>> {
>>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
>>> switch (event) {
>>> case CHR_EVENT_OPENED:
>>> vm_stop(VMSTOP_USER);
>>> + vm_status_set(VMST_DEBUG);
>>> gdb_has_xml = 0;
>>> break;
>>> default:
>>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
>>> {
>>> if (vm_running) {
>>> vm_stop(VMSTOP_USER);
>>> + vm_status_set(VMST_DEBUG);
>>> }
>>> }
>>> #endif
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index ca17a43..bf9df41 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>>> s->bus->error_status = op;
>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>> vm_stop(VMSTOP_DISKFULL);
>>> + vm_status_set(VMST_IOERROR);
>>> } else {
>>> if (op& BM_STATUS_DMA_RETRY) {
>>> dma_buf_commit(s, 0);
>>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>>> index a8c7372..66037fd 100644
>>> --- a/hw/scsi-disk.c
>>> +++ b/hw/scsi-disk.c
>>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>>>
>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>> vm_stop(VMSTOP_DISKFULL);
>>> + vm_status_set(VMST_IOERROR);
>>> } else {
>>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
>>> scsi_req_data(&r->req, 0);
>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>> index 91e0394..bf70200 100644
>>> --- a/hw/virtio-blk.c
>>> +++ b/hw/virtio-blk.c
>>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>>> s->rq = req;
>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>> vm_stop(VMSTOP_DISKFULL);
>>> + vm_status_set(VMST_IOERROR);
>>> } else {
>>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
>>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
>>> diff --git a/hw/watchdog.c b/hw/watchdog.c
>>> index 1c900a1..d130cbb 100644
>>> --- a/hw/watchdog.c
>>> +++ b/hw/watchdog.c
>>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
>>> case WDT_PAUSE: /* same as 'stop' command in monitor */
>>> watchdog_mon_event("pause");
>>> vm_stop(VMSTOP_WATCHDOG);
>>> + vm_status_set(VMST_WATCHDOG);
>>> break;
>>>
>>> case WDT_DEBUG:
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index cbc2532..aee9e0a 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
>>> if (ret< 0) {
>>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
>>> vm_stop(VMSTOP_PANIC);
>>> + vm_status_set(VMST_INTERROR);
>>> }
>>>
>>> env->exit_request = 0;
>>> diff --git a/migration.c b/migration.c
>>> index af3a1f2..674792f 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
>>> }
>>> state = MIG_STATE_ERROR;
>>> }
>>> + if (state == MIG_STATE_COMPLETED) {
>>> + vm_status_set(VMST_POSTMIGRATE);
>>> + }
>>> s->state = state;
>>> notifier_list_notify(&migration_state_notifiers);
>>> }
>>> diff --git a/monitor.c b/monitor.c
>>> index 67ceb46..1cb3191 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
>>> return 0;
>>> }
>>>
>>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>>>
>>> vm_stop(VMSTOP_LOADVM);
>>>
>>> - if (load_vmstate(name) == 0&& saved_vm_running) {
>>> + if (load_vmstate(name)< 0) {
>>> + vm_status_set(VMST_LOADERROR);
>>> + } else if (saved_vm_running) {
>>> vm_start();
>>> }
>>> }
>>> diff --git a/sysemu.h b/sysemu.h
>>> index d3013f5..7308ac5 100644
>>> --- a/sysemu.h
>>> +++ b/sysemu.h
>>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
>>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
>>> int qemu_loadvm_state(QEMUFile *f);
>>>
>>> +typedef enum {
>>> + VMST_NOSTATUS,
>>> + VMST_DEBUG,
>>> + VMST_INMIGRATE,
>>> + VMST_POSTMIGRATE,
>>> + VMST_INTERROR,
>>> + VMST_IOERROR,
>>> + VMST_LOADERROR,
>>> + VMST_PAUSED,
>>> + VMST_PRELAUNCH,
>>> + VMST_RUNNING,
>>> + VMST_SHUTDOWN,
>>> + VMST_WATCHDOG,
>>> + VMST_MAX,
>>> +} VMStatus;
>>> +
>>> +void vm_status_set(VMStatus s);
>>> +const char *vm_status_get_name(void);
>>> +
>>> /* SLIRP */
>>> void do_info_slirp(Monitor *mon);
>>>
>>> diff --git a/vl.c b/vl.c
>>> index fcd7395..fb7208f 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
>>> }
>>>
>>> /***********************************************************/
>>> +/* VMStatus */
>>> +
>>> +static const char *const vm_status_name[VMST_MAX] = {
>>> + [VMST_DEBUG] = "debug",
>>> + [VMST_INMIGRATE] = "inmigrate",
>>> + [VMST_POSTMIGRATE] = "postmigrate",
>>> + [VMST_INTERROR] = "internal-error",
>>> + [VMST_IOERROR] = "io-error",
>>> + [VMST_LOADERROR] = "load-state-error",
>>> + [VMST_PAUSED] = "paused",
>>> + [VMST_PRELAUNCH] = "prelaunch",
>>> + [VMST_RUNNING] = "running",
>>> + [VMST_SHUTDOWN] = "shutdown",
>>> + [VMST_WATCHDOG] = "watchdog-error",
>>> +};
>>> +
>>> +static VMStatus qemu_vm_status = VMST_NOSTATUS;
>>> +
>>> +void vm_status_set(VMStatus s)
>>> +{
>>> + assert(s> VMST_NOSTATUS&& s< VMST_MAX);
>>> + qemu_vm_status = s;
>>> +}
>>> +
>>> +const char *vm_status_get_name(void)
>>> +{
>>> + assert(qemu_vm_status> VMST_NOSTATUS&& qemu_vm_status< VMST_MAX);
>>> + return vm_status_name[qemu_vm_status];
>>> +}
>>> +
>>> +/***********************************************************/
>>> /* real time host monotonic timer */
>>>
>>> /***********************************************************/
>>> @@ -1150,6 +1181,7 @@ void vm_start(void)
>>> vm_state_notify(1, 0);
>>> resume_all_vcpus();
>>> monitor_protocol_event(QEVENT_RESUME, NULL);
>>> + vm_status_set(VMST_RUNNING);
>>> }
>>> }
>>>
>>> @@ -1392,12 +1424,14 @@ static void main_loop(void)
>>>
>>> if (qemu_debug_requested()) {
>>> vm_stop(VMSTOP_DEBUG);
>>> + vm_status_set(VMST_DEBUG);
>>> }
>>> if (qemu_shutdown_requested()) {
>>> qemu_kill_report();
>>> monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
>>> if (no_shutdown) {
>>> vm_stop(VMSTOP_SHUTDOWN);
>>> + vm_status_set(VMST_SHUTDOWN);
>>> no_shutdown = 0;
>>> } else
>>> break;
>>> @@ -2476,6 +2510,7 @@ int main(int argc, char **argv, char **envp)
>>> break;
>>> case QEMU_OPTION_S:
>>> autostart = 0;
>>> + vm_status_set(VMST_PRELAUNCH);
>>> break;
>>> case QEMU_OPTION_k:
>>> keyboard_layout = optarg;
>>> @@ -3307,11 +3342,13 @@ int main(int argc, char **argv, char **envp)
>>> qemu_system_reset(VMRESET_SILENT);
>>> if (loadvm) {
>>> if (load_vmstate(loadvm)< 0) {
>>> + vm_status_set(VMST_LOADERROR);
>>> autostart = 0;
>>> }
>>> }
>>>
>>> if (incoming) {
>>> + vm_status_set(VMST_INMIGRATE);
>>> int ret = qemu_start_incoming_migration(incoming);
>>> if (ret< 0) {
>>> fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
>>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-05 18:58 ` Anthony Liguori
@ 2011-07-05 19:34 ` Luiz Capitulino
0 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 19:34 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kwolf, jan.kiszka, qemu-devel, armbru, stefanha, jdenemar
On Tue, 05 Jul 2011 13:58:34 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 07/05/2011 01:51 PM, Luiz Capitulino wrote:
> > On Tue, 05 Jul 2011 13:33:07 -0500
> > Anthony Liguori<anthony@codemonkey.ws> wrote:
> >
> >> On 07/05/2011 01:17 PM, Luiz Capitulino wrote:
> >>> We need to track the VM status so that QMP can report it to clients.
> >>>
> >>> This commit adds the VMStatus type and related functions. The
> >>> vm_status_set() function is used to keep track of the current
> >>> VM status.
> >>>
> >>> The current statuses are:
> >>
> >> Which states are terminal, and what's the proper procedure to recover
> >> from non-terminal states? Is it cont or system_reset followed by cont?
> >
> > Can I add that to the next patch (which also adds the QMP doc) or do you
> > prefer it in this patch?
>
> It can be a separate patch, I was more curious about whether you had
> thought through this for each of the states so that the states we were
> introducing were well defined.
I haven't to be honest. Adding it as documentation will be a good
exercise though. Will do that for v2, but if you have comments about
specific states, please, let me know now.
>
> For instance, it's not clear to me what the semantics of
> 'load-state-error' are and how it's different from prelaunch.
>
> I'm quite surprised that 'load-state-error' wouldn't be a terminal error.
Me too. I thought that starting (but not running) the VM after a failed
load_vmstate() was a feature to allow debugging. I've never thought that it
were possible to do 'cont' in this case (only badness can happen when doing
this, no?).
There are two solutions:
1. Just drop load-state-error
2. Doesn't allow a VM that failed a load_vmstate() call to be put to run,
unless another call to load_vmstate() succeeds
>
> Regards,
>
> Anthony Liguori
>
> >
> >>
> >>>
> >>> - debug: guest is running under gdb
> >>> - inmigrate: guest is paused waiting for an incoming migration
> >>> - postmigrate: guest is paused following a successful migration
> >>> - internal-error: Fatal internal error that prevents further guest
> >>> execution
> >>> - load-state-error: guest is paused following a failed 'loadvm'
> >>> - io-error: the last IOP has failed and the device is configured
> >>> to pause on I/O errors
> >>> - watchdog-error: the watchdog action is configured to pause and
> >>> has been triggered
> >>> - paused: guest has been paused via the 'stop' command
> >>> - prelaunch: QEMU was started with -S and guest has not started
> >>> - running: guest is actively running
> >>> - shutdown: guest is shut down (and -no-shutdown is in use)
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> >>
> >>>
> >>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >>> ---
> >>> gdbstub.c | 4 ++++
> >>> hw/ide/core.c | 1 +
> >>> hw/scsi-disk.c | 1 +
> >>> hw/virtio-blk.c | 1 +
> >>> hw/watchdog.c | 1 +
> >>> kvm-all.c | 1 +
> >>> migration.c | 3 +++
> >>> monitor.c | 5 ++++-
> >>> sysemu.h | 19 +++++++++++++++++++
> >>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> >>> 10 files changed, 72 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/gdbstub.c b/gdbstub.c
> >>> index c085a5a..61b700a 100644
> >>> --- a/gdbstub.c
> >>> +++ b/gdbstub.c
> >>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> >>> s->state = RS_SYSCALL;
> >>> #ifndef CONFIG_USER_ONLY
> >>> vm_stop(VMSTOP_DEBUG);
> >>> + vm_status_set(VMST_DEBUG);
> >>> #endif
> >>> s->state = RS_IDLE;
> >>> va_start(va, fmt);
> >>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> >>> /* when the CPU is running, we cannot do anything except stop
> >>> it when receiving a char */
> >>> vm_stop(VMSTOP_USER);
> >>> + vm_status_set(VMST_DEBUG);
> >>> } else
> >>> #endif
> >>> {
> >>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> >>> switch (event) {
> >>> case CHR_EVENT_OPENED:
> >>> vm_stop(VMSTOP_USER);
> >>> + vm_status_set(VMST_DEBUG);
> >>> gdb_has_xml = 0;
> >>> break;
> >>> default:
> >>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> >>> {
> >>> if (vm_running) {
> >>> vm_stop(VMSTOP_USER);
> >>> + vm_status_set(VMST_DEBUG);
> >>> }
> >>> }
> >>> #endif
> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >>> index ca17a43..bf9df41 100644
> >>> --- a/hw/ide/core.c
> >>> +++ b/hw/ide/core.c
> >>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> >>> s->bus->error_status = op;
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>> vm_stop(VMSTOP_DISKFULL);
> >>> + vm_status_set(VMST_IOERROR);
> >>> } else {
> >>> if (op& BM_STATUS_DMA_RETRY) {
> >>> dma_buf_commit(s, 0);
> >>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> >>> index a8c7372..66037fd 100644
> >>> --- a/hw/scsi-disk.c
> >>> +++ b/hw/scsi-disk.c
> >>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> >>>
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>> vm_stop(VMSTOP_DISKFULL);
> >>> + vm_status_set(VMST_IOERROR);
> >>> } else {
> >>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
> >>> scsi_req_data(&r->req, 0);
> >>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> >>> index 91e0394..bf70200 100644
> >>> --- a/hw/virtio-blk.c
> >>> +++ b/hw/virtio-blk.c
> >>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> >>> s->rq = req;
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>> vm_stop(VMSTOP_DISKFULL);
> >>> + vm_status_set(VMST_IOERROR);
> >>> } else {
> >>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> >>> diff --git a/hw/watchdog.c b/hw/watchdog.c
> >>> index 1c900a1..d130cbb 100644
> >>> --- a/hw/watchdog.c
> >>> +++ b/hw/watchdog.c
> >>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> >>> case WDT_PAUSE: /* same as 'stop' command in monitor */
> >>> watchdog_mon_event("pause");
> >>> vm_stop(VMSTOP_WATCHDOG);
> >>> + vm_status_set(VMST_WATCHDOG);
> >>> break;
> >>>
> >>> case WDT_DEBUG:
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index cbc2532..aee9e0a 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> >>> if (ret< 0) {
> >>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> >>> vm_stop(VMSTOP_PANIC);
> >>> + vm_status_set(VMST_INTERROR);
> >>> }
> >>>
> >>> env->exit_request = 0;
> >>> diff --git a/migration.c b/migration.c
> >>> index af3a1f2..674792f 100644
> >>> --- a/migration.c
> >>> +++ b/migration.c
> >>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> >>> }
> >>> state = MIG_STATE_ERROR;
> >>> }
> >>> + if (state == MIG_STATE_COMPLETED) {
> >>> + vm_status_set(VMST_POSTMIGRATE);
> >>> + }
> >>> s->state = state;
> >>> notifier_list_notify(&migration_state_notifiers);
> >>> }
> >>> diff --git a/monitor.c b/monitor.c
> >>> index 67ceb46..1cb3191 100644
> >>> --- a/monitor.c
> >>> +++ b/monitor.c
> >>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> >>> return 0;
> >>> }
> >>>
> >>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
> >>>
> >>> vm_stop(VMSTOP_LOADVM);
> >>>
> >>> - if (load_vmstate(name) == 0&& saved_vm_running) {
> >>> + if (load_vmstate(name)< 0) {
> >>> + vm_status_set(VMST_LOADERROR);
> >>> + } else if (saved_vm_running) {
> >>> vm_start();
> >>> }
> >>> }
> >>> diff --git a/sysemu.h b/sysemu.h
> >>> index d3013f5..7308ac5 100644
> >>> --- a/sysemu.h
> >>> +++ b/sysemu.h
> >>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> >>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> >>> int qemu_loadvm_state(QEMUFile *f);
> >>>
> >>> +typedef enum {
> >>> + VMST_NOSTATUS,
> >>> + VMST_DEBUG,
> >>> + VMST_INMIGRATE,
> >>> + VMST_POSTMIGRATE,
> >>> + VMST_INTERROR,
> >>> + VMST_IOERROR,
> >>> + VMST_LOADERROR,
> >>> + VMST_PAUSED,
> >>> + VMST_PRELAUNCH,
> >>> + VMST_RUNNING,
> >>> + VMST_SHUTDOWN,
> >>> + VMST_WATCHDOG,
> >>> + VMST_MAX,
> >>> +} VMStatus;
> >>> +
> >>> +void vm_status_set(VMStatus s);
> >>> +const char *vm_status_get_name(void);
> >>> +
> >>> /* SLIRP */
> >>> void do_info_slirp(Monitor *mon);
> >>>
> >>> diff --git a/vl.c b/vl.c
> >>> index fcd7395..fb7208f 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> >>> }
> >>>
> >>> /***********************************************************/
> >>> +/* VMStatus */
> >>> +
> >>> +static const char *const vm_status_name[VMST_MAX] = {
> >>> + [VMST_DEBUG] = "debug",
> >>> + [VMST_INMIGRATE] = "inmigrate",
> >>> + [VMST_POSTMIGRATE] = "postmigrate",
> >>> + [VMST_INTERROR] = "internal-error",
> >>> + [VMST_IOERROR] = "io-error",
> >>> + [VMST_LOADERROR] = "load-state-error",
> >>> + [VMST_PAUSED] = "paused",
> >>> + [VMST_PRELAUNCH] = "prelaunch",
> >>> + [VMST_RUNNING] = "running",
> >>> + [VMST_SHUTDOWN] = "shutdown",
> >>> + [VMST_WATCHDOG] = "watchdog-error",
> >>> +};
> >>> +
> >>> +static VMStatus qemu_vm_status = VMST_NOSTATUS;
> >>> +
> >>> +void vm_status_set(VMStatus s)
> >>> +{
> >>> + assert(s> VMST_NOSTATUS&& s< VMST_MAX);
> >>> + qemu_vm_status = s;
> >>> +}
> >>> +
> >>> +const char *vm_status_get_name(void)
> >>> +{
> >>> + assert(qemu_vm_status> VMST_NOSTATUS&& qemu_vm_status< VMST_MAX);
> >>> + return vm_status_name[qemu_vm_status];
> >>> +}
> >>> +
> >>> +/***********************************************************/
> >>> /* real time host monotonic timer */
> >>>
> >>> /***********************************************************/
> >>> @@ -1150,6 +1181,7 @@ void vm_start(void)
> >>> vm_state_notify(1, 0);
> >>> resume_all_vcpus();
> >>> monitor_protocol_event(QEVENT_RESUME, NULL);
> >>> + vm_status_set(VMST_RUNNING);
> >>> }
> >>> }
> >>>
> >>> @@ -1392,12 +1424,14 @@ static void main_loop(void)
> >>>
> >>> if (qemu_debug_requested()) {
> >>> vm_stop(VMSTOP_DEBUG);
> >>> + vm_status_set(VMST_DEBUG);
> >>> }
> >>> if (qemu_shutdown_requested()) {
> >>> qemu_kill_report();
> >>> monitor_protocol_event(QEVENT_SHUTDOWN, NULL);
> >>> if (no_shutdown) {
> >>> vm_stop(VMSTOP_SHUTDOWN);
> >>> + vm_status_set(VMST_SHUTDOWN);
> >>> no_shutdown = 0;
> >>> } else
> >>> break;
> >>> @@ -2476,6 +2510,7 @@ int main(int argc, char **argv, char **envp)
> >>> break;
> >>> case QEMU_OPTION_S:
> >>> autostart = 0;
> >>> + vm_status_set(VMST_PRELAUNCH);
> >>> break;
> >>> case QEMU_OPTION_k:
> >>> keyboard_layout = optarg;
> >>> @@ -3307,11 +3342,13 @@ int main(int argc, char **argv, char **envp)
> >>> qemu_system_reset(VMRESET_SILENT);
> >>> if (loadvm) {
> >>> if (load_vmstate(loadvm)< 0) {
> >>> + vm_status_set(VMST_LOADERROR);
> >>> autostart = 0;
> >>> }
> >>> }
> >>>
> >>> if (incoming) {
> >>> + vm_status_set(VMST_INMIGRATE);
> >>> int ret = qemu_start_incoming_migration(incoming);
> >>> if (ret< 0) {
> >>> fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
> >>
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-05 18:17 ` [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type Luiz Capitulino
2011-07-05 18:33 ` Anthony Liguori
@ 2011-07-12 7:28 ` Markus Armbruster
2011-07-12 14:25 ` Luiz Capitulino
1 sibling, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2011-07-12 7:28 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, jan.kiszka, jdenemar, qemu-devel, stefanha
Luiz Capitulino <lcapitulino@redhat.com> writes:
> We need to track the VM status so that QMP can report it to clients.
>
> This commit adds the VMStatus type and related functions. The
> vm_status_set() function is used to keep track of the current
> VM status.
>
> The current statuses are:
Nitpicking about names, bear with me.
> - debug: guest is running under gdb
> - inmigrate: guest is paused waiting for an incoming migration
incoming-migration?
> - postmigrate: guest is paused following a successful migration
post-migrate?
> - internal-error: Fatal internal error that prevents further guest
> execution
> - load-state-error: guest is paused following a failed 'loadvm'
Less than obvious. If you like concrete, name it loadvm-failed. If you
like abstract, name it restore-vm-failed.
> - io-error: the last IOP has failed and the device is configured
> to pause on I/O errors
> - watchdog-error: the watchdog action is configured to pause and
> has been triggered
Sounds like the watchdog suffered an error. watchdog-fired?
> - paused: guest has been paused via the 'stop' command
stop-command?
> - prelaunch: QEMU was started with -S and guest has not started
unstarted?
> - running: guest is actively running
> - shutdown: guest is shut down (and -no-shutdown is in use)
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> gdbstub.c | 4 ++++
> hw/ide/core.c | 1 +
> hw/scsi-disk.c | 1 +
> hw/virtio-blk.c | 1 +
> hw/watchdog.c | 1 +
> kvm-all.c | 1 +
> migration.c | 3 +++
> monitor.c | 5 ++++-
> sysemu.h | 19 +++++++++++++++++++
> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> 10 files changed, 72 insertions(+), 1 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index c085a5a..61b700a 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> s->state = RS_SYSCALL;
> #ifndef CONFIG_USER_ONLY
> vm_stop(VMSTOP_DEBUG);
> + vm_status_set(VMST_DEBUG);
> #endif
> s->state = RS_IDLE;
> va_start(va, fmt);
> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> /* when the CPU is running, we cannot do anything except stop
> it when receiving a char */
> vm_stop(VMSTOP_USER);
> + vm_status_set(VMST_DEBUG);
> } else
> #endif
> {
> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> switch (event) {
> case CHR_EVENT_OPENED:
> vm_stop(VMSTOP_USER);
> + vm_status_set(VMST_DEBUG);
> gdb_has_xml = 0;
> break;
> default:
Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> {
> if (vm_running) {
> vm_stop(VMSTOP_USER);
> + vm_status_set(VMST_DEBUG);
> }
> }
> #endif
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index ca17a43..bf9df41 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> s->bus->error_status = op;
> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> vm_stop(VMSTOP_DISKFULL);
> + vm_status_set(VMST_IOERROR);
> } else {
> if (op & BM_STATUS_DMA_RETRY) {
> dma_buf_commit(s, 0);
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index a8c7372..66037fd 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>
> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> vm_stop(VMSTOP_DISKFULL);
> + vm_status_set(VMST_IOERROR);
> } else {
> if (type == SCSI_REQ_STATUS_RETRY_READ) {
> scsi_req_data(&r->req, 0);
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 91e0394..bf70200 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> s->rq = req;
> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> vm_stop(VMSTOP_DISKFULL);
> + vm_status_set(VMST_IOERROR);
> } else {
> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> diff --git a/hw/watchdog.c b/hw/watchdog.c
> index 1c900a1..d130cbb 100644
> --- a/hw/watchdog.c
> +++ b/hw/watchdog.c
> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> case WDT_PAUSE: /* same as 'stop' command in monitor */
> watchdog_mon_event("pause");
> vm_stop(VMSTOP_WATCHDOG);
> + vm_status_set(VMST_WATCHDOG);
> break;
>
> case WDT_DEBUG:
> diff --git a/kvm-all.c b/kvm-all.c
> index cbc2532..aee9e0a 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> if (ret < 0) {
> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> vm_stop(VMSTOP_PANIC);
> + vm_status_set(VMST_INTERROR);
> }
>
> env->exit_request = 0;
> diff --git a/migration.c b/migration.c
> index af3a1f2..674792f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> }
> state = MIG_STATE_ERROR;
> }
> + if (state == MIG_STATE_COMPLETED) {
> + vm_status_set(VMST_POSTMIGRATE);
> + }
> s->state = state;
> notifier_list_notify(&migration_state_notifiers);
> }
> diff --git a/monitor.c b/monitor.c
> index 67ceb46..1cb3191 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> return 0;
> }
>
> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>
> vm_stop(VMSTOP_LOADVM);
>
> - if (load_vmstate(name) == 0 && saved_vm_running) {
> + if (load_vmstate(name) < 0) {
> + vm_status_set(VMST_LOADERROR);
> + } else if (saved_vm_running) {
> vm_start();
> }
> }
> diff --git a/sysemu.h b/sysemu.h
> index d3013f5..7308ac5 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> int qemu_loadvm_state(QEMUFile *f);
>
> +typedef enum {
> + VMST_NOSTATUS,
> + VMST_DEBUG,
> + VMST_INMIGRATE,
> + VMST_POSTMIGRATE,
> + VMST_INTERROR,
> + VMST_IOERROR,
> + VMST_LOADERROR,
> + VMST_PAUSED,
> + VMST_PRELAUNCH,
> + VMST_RUNNING,
> + VMST_SHUTDOWN,
> + VMST_WATCHDOG,
> + VMST_MAX,
> +} VMStatus;
How are these related to the VMSTOP_*?
Why do we need a separate enumeration?
> +
> +void vm_status_set(VMStatus s);
> +const char *vm_status_get_name(void);
> +
> /* SLIRP */
> void do_info_slirp(Monitor *mon);
>
> diff --git a/vl.c b/vl.c
> index fcd7395..fb7208f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> }
>
> /***********************************************************/
> +/* VMStatus */
> +
> +static const char *const vm_status_name[VMST_MAX] = {
> + [VMST_DEBUG] = "debug",
> + [VMST_INMIGRATE] = "inmigrate",
> + [VMST_POSTMIGRATE] = "postmigrate",
> + [VMST_INTERROR] = "internal-error",
> + [VMST_IOERROR] = "io-error",
> + [VMST_LOADERROR] = "load-state-error",
> + [VMST_PAUSED] = "paused",
> + [VMST_PRELAUNCH] = "prelaunch",
> + [VMST_RUNNING] = "running",
> + [VMST_SHUTDOWN] = "shutdown",
> + [VMST_WATCHDOG] = "watchdog-error",
> +};
> +
> +static VMStatus qemu_vm_status = VMST_NOSTATUS;
> +
> +void vm_status_set(VMStatus s)
> +{
> + assert(s > VMST_NOSTATUS && s < VMST_MAX);
> + qemu_vm_status = s;
> +}
> +
> +const char *vm_status_get_name(void)
> +{
> + assert(qemu_vm_status > VMST_NOSTATUS && qemu_vm_status < VMST_MAX);
> + return vm_status_name[qemu_vm_status];
> +}
> +
Couldn't find a better place than the ol' vl.c rubbish dump ;)
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-12 7:28 ` Markus Armbruster
@ 2011-07-12 14:25 ` Luiz Capitulino
2011-07-12 14:51 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 14:25 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, jan.kiszka, jdenemar, qemu-devel, stefanha
On Tue, 12 Jul 2011 09:28:05 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> > We need to track the VM status so that QMP can report it to clients.
> >
> > This commit adds the VMStatus type and related functions. The
> > vm_status_set() function is used to keep track of the current
> > VM status.
> >
> > The current statuses are:
>
> Nitpicking about names, bear with me.
>
> > - debug: guest is running under gdb
> > - inmigrate: guest is paused waiting for an incoming migration
>
> incoming-migration?
>
> > - postmigrate: guest is paused following a successful migration
>
> post-migrate?
>
> > - internal-error: Fatal internal error that prevents further guest
> > execution
> > - load-state-error: guest is paused following a failed 'loadvm'
>
> Less than obvious. If you like concrete, name it loadvm-failed. If you
> like abstract, name it restore-vm-failed.
Ok for your suggestions above.
>
> > - io-error: the last IOP has failed and the device is configured
> > to pause on I/O errors
> > - watchdog-error: the watchdog action is configured to pause and
> > has been triggered
>
> Sounds like the watchdog suffered an error. watchdog-fired?
Maybe watchdog-paused.
>
> > - paused: guest has been paused via the 'stop' command
>
> stop-command?
I prefer 'paused', it communicates better the state we're in.
>
> > - prelaunch: QEMU was started with -S and guest has not started
>
> unstarted?
Looks the same to me.
>
> > - running: guest is actively running
> > - shutdown: guest is shut down (and -no-shutdown is in use)
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> > gdbstub.c | 4 ++++
> > hw/ide/core.c | 1 +
> > hw/scsi-disk.c | 1 +
> > hw/virtio-blk.c | 1 +
> > hw/watchdog.c | 1 +
> > kvm-all.c | 1 +
> > migration.c | 3 +++
> > monitor.c | 5 ++++-
> > sysemu.h | 19 +++++++++++++++++++
> > vl.c | 37 +++++++++++++++++++++++++++++++++++++
> > 10 files changed, 72 insertions(+), 1 deletions(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index c085a5a..61b700a 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> > s->state = RS_SYSCALL;
> > #ifndef CONFIG_USER_ONLY
> > vm_stop(VMSTOP_DEBUG);
> > + vm_status_set(VMST_DEBUG);
> > #endif
> > s->state = RS_IDLE;
> > va_start(va, fmt);
> > @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> > /* when the CPU is running, we cannot do anything except stop
> > it when receiving a char */
> > vm_stop(VMSTOP_USER);
> > + vm_status_set(VMST_DEBUG);
> > } else
> > #endif
> > {
> > @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> > switch (event) {
> > case CHR_EVENT_OPENED:
> > vm_stop(VMSTOP_USER);
> > + vm_status_set(VMST_DEBUG);
> > gdb_has_xml = 0;
> > break;
> > default:
>
> Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
>
> > @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> > {
> > if (vm_running) {
> > vm_stop(VMSTOP_USER);
> > + vm_status_set(VMST_DEBUG);
> > }
> > }
> > #endif
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index ca17a43..bf9df41 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> > s->bus->error_status = op;
> > bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > vm_stop(VMSTOP_DISKFULL);
> > + vm_status_set(VMST_IOERROR);
> > } else {
> > if (op & BM_STATUS_DMA_RETRY) {
> > dma_buf_commit(s, 0);
> > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> > index a8c7372..66037fd 100644
> > --- a/hw/scsi-disk.c
> > +++ b/hw/scsi-disk.c
> > @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> >
> > bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > vm_stop(VMSTOP_DISKFULL);
> > + vm_status_set(VMST_IOERROR);
> > } else {
> > if (type == SCSI_REQ_STATUS_RETRY_READ) {
> > scsi_req_data(&r->req, 0);
> > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > index 91e0394..bf70200 100644
> > --- a/hw/virtio-blk.c
> > +++ b/hw/virtio-blk.c
> > @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> > s->rq = req;
> > bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > vm_stop(VMSTOP_DISKFULL);
> > + vm_status_set(VMST_IOERROR);
> > } else {
> > virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> > bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> > diff --git a/hw/watchdog.c b/hw/watchdog.c
> > index 1c900a1..d130cbb 100644
> > --- a/hw/watchdog.c
> > +++ b/hw/watchdog.c
> > @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> > case WDT_PAUSE: /* same as 'stop' command in monitor */
> > watchdog_mon_event("pause");
> > vm_stop(VMSTOP_WATCHDOG);
> > + vm_status_set(VMST_WATCHDOG);
> > break;
> >
> > case WDT_DEBUG:
> > diff --git a/kvm-all.c b/kvm-all.c
> > index cbc2532..aee9e0a 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> > if (ret < 0) {
> > cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> > vm_stop(VMSTOP_PANIC);
> > + vm_status_set(VMST_INTERROR);
> > }
> >
> > env->exit_request = 0;
> > diff --git a/migration.c b/migration.c
> > index af3a1f2..674792f 100644
> > --- a/migration.c
> > +++ b/migration.c
> > @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> > }
> > state = MIG_STATE_ERROR;
> > }
> > + if (state == MIG_STATE_COMPLETED) {
> > + vm_status_set(VMST_POSTMIGRATE);
> > + }
> > s->state = state;
> > notifier_list_notify(&migration_state_notifiers);
> > }
> > diff --git a/monitor.c b/monitor.c
> > index 67ceb46..1cb3191 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> > return 0;
> > }
> >
> > @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
> >
> > vm_stop(VMSTOP_LOADVM);
> >
> > - if (load_vmstate(name) == 0 && saved_vm_running) {
> > + if (load_vmstate(name) < 0) {
> > + vm_status_set(VMST_LOADERROR);
> > + } else if (saved_vm_running) {
> > vm_start();
> > }
> > }
> > diff --git a/sysemu.h b/sysemu.h
> > index d3013f5..7308ac5 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> > void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> > int qemu_loadvm_state(QEMUFile *f);
> >
> > +typedef enum {
> > + VMST_NOSTATUS,
> > + VMST_DEBUG,
> > + VMST_INMIGRATE,
> > + VMST_POSTMIGRATE,
> > + VMST_INTERROR,
> > + VMST_IOERROR,
> > + VMST_LOADERROR,
> > + VMST_PAUSED,
> > + VMST_PRELAUNCH,
> > + VMST_RUNNING,
> > + VMST_SHUTDOWN,
> > + VMST_WATCHDOG,
> > + VMST_MAX,
> > +} VMStatus;
>
> How are these related to the VMSTOP_*?
>
> Why do we need a separate enumeration?
>
> > +
> > +void vm_status_set(VMStatus s);
> > +const char *vm_status_get_name(void);
> > +
> > /* SLIRP */
> > void do_info_slirp(Monitor *mon);
> >
> > diff --git a/vl.c b/vl.c
> > index fcd7395..fb7208f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -309,6 +309,37 @@ static int default_driver_check(QemuOpts *opts, void *opaque)
> > }
> >
> > /***********************************************************/
> > +/* VMStatus */
> > +
> > +static const char *const vm_status_name[VMST_MAX] = {
> > + [VMST_DEBUG] = "debug",
> > + [VMST_INMIGRATE] = "inmigrate",
> > + [VMST_POSTMIGRATE] = "postmigrate",
> > + [VMST_INTERROR] = "internal-error",
> > + [VMST_IOERROR] = "io-error",
> > + [VMST_LOADERROR] = "load-state-error",
> > + [VMST_PAUSED] = "paused",
> > + [VMST_PRELAUNCH] = "prelaunch",
> > + [VMST_RUNNING] = "running",
> > + [VMST_SHUTDOWN] = "shutdown",
> > + [VMST_WATCHDOG] = "watchdog-error",
> > +};
> > +
> > +static VMStatus qemu_vm_status = VMST_NOSTATUS;
> > +
> > +void vm_status_set(VMStatus s)
> > +{
> > + assert(s > VMST_NOSTATUS && s < VMST_MAX);
> > + qemu_vm_status = s;
> > +}
> > +
> > +const char *vm_status_get_name(void)
> > +{
> > + assert(qemu_vm_status > VMST_NOSTATUS && qemu_vm_status < VMST_MAX);
> > + return vm_status_name[qemu_vm_status];
> > +}
> > +
>
> Couldn't find a better place than the ol' vl.c rubbish dump ;)
>
> [...]
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-12 14:25 ` Luiz Capitulino
@ 2011-07-12 14:51 ` Kevin Wolf
2011-07-12 15:12 ` Luiz Capitulino
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2011-07-12 14:51 UTC (permalink / raw)
To: Luiz Capitulino
Cc: stefanha, jan.kiszka, jdenemar, Markus Armbruster, qemu-devel
Am 12.07.2011 16:25, schrieb Luiz Capitulino:
> On Tue, 12 Jul 2011 09:28:05 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>
>>> We need to track the VM status so that QMP can report it to clients.
>>>
>>> This commit adds the VMStatus type and related functions. The
>>> vm_status_set() function is used to keep track of the current
>>> VM status.
>>>
>>> The current statuses are:
>>
>> Nitpicking about names, bear with me.
>>
>>> - debug: guest is running under gdb
>>> - inmigrate: guest is paused waiting for an incoming migration
>>
>> incoming-migration?
>>
>>> - postmigrate: guest is paused following a successful migration
>>
>> post-migrate?
>>
>>> - internal-error: Fatal internal error that prevents further guest
>>> execution
>>> - load-state-error: guest is paused following a failed 'loadvm'
>>
>> Less than obvious. If you like concrete, name it loadvm-failed. If you
>> like abstract, name it restore-vm-failed.
>
> Ok for your suggestions above.
>
>>
>>> - io-error: the last IOP has failed and the device is configured
>>> to pause on I/O errors
>>> - watchdog-error: the watchdog action is configured to pause and
>>> has been triggered
>>
>> Sounds like the watchdog suffered an error. watchdog-fired?
>
> Maybe watchdog-paused.
>
>>
>>> - paused: guest has been paused via the 'stop' command
>>
>> stop-command?
>
> I prefer 'paused', it communicates better the state we're in.
>
>>
>>> - prelaunch: QEMU was started with -S and guest has not started
>>
>> unstarted?
>
> Looks the same to me.
>
>>
>>> - running: guest is actively running
>>> - shutdown: guest is shut down (and -no-shutdown is in use)
>>>
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>> gdbstub.c | 4 ++++
>>> hw/ide/core.c | 1 +
>>> hw/scsi-disk.c | 1 +
>>> hw/virtio-blk.c | 1 +
>>> hw/watchdog.c | 1 +
>>> kvm-all.c | 1 +
>>> migration.c | 3 +++
>>> monitor.c | 5 ++++-
>>> sysemu.h | 19 +++++++++++++++++++
>>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
>>> 10 files changed, 72 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index c085a5a..61b700a 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
>>> s->state = RS_SYSCALL;
>>> #ifndef CONFIG_USER_ONLY
>>> vm_stop(VMSTOP_DEBUG);
>>> + vm_status_set(VMST_DEBUG);
>>> #endif
>>> s->state = RS_IDLE;
>>> va_start(va, fmt);
>>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>>> /* when the CPU is running, we cannot do anything except stop
>>> it when receiving a char */
>>> vm_stop(VMSTOP_USER);
>>> + vm_status_set(VMST_DEBUG);
>>> } else
>>> #endif
>>> {
>>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
>>> switch (event) {
>>> case CHR_EVENT_OPENED:
>>> vm_stop(VMSTOP_USER);
>>> + vm_status_set(VMST_DEBUG);
>>> gdb_has_xml = 0;
>>> break;
>>> default:
>>
>> Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
>>
>>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
>>> {
>>> if (vm_running) {
>>> vm_stop(VMSTOP_USER);
>>> + vm_status_set(VMST_DEBUG);
>>> }
>>> }
>>> #endif
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index ca17a43..bf9df41 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>>> s->bus->error_status = op;
>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>> vm_stop(VMSTOP_DISKFULL);
>>> + vm_status_set(VMST_IOERROR);
>>> } else {
>>> if (op & BM_STATUS_DMA_RETRY) {
>>> dma_buf_commit(s, 0);
>>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>>> index a8c7372..66037fd 100644
>>> --- a/hw/scsi-disk.c
>>> +++ b/hw/scsi-disk.c
>>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>>>
>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>> vm_stop(VMSTOP_DISKFULL);
>>> + vm_status_set(VMST_IOERROR);
>>> } else {
>>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
>>> scsi_req_data(&r->req, 0);
>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>> index 91e0394..bf70200 100644
>>> --- a/hw/virtio-blk.c
>>> +++ b/hw/virtio-blk.c
>>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>>> s->rq = req;
>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>> vm_stop(VMSTOP_DISKFULL);
>>> + vm_status_set(VMST_IOERROR);
>>> } else {
>>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
>>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
>>> diff --git a/hw/watchdog.c b/hw/watchdog.c
>>> index 1c900a1..d130cbb 100644
>>> --- a/hw/watchdog.c
>>> +++ b/hw/watchdog.c
>>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
>>> case WDT_PAUSE: /* same as 'stop' command in monitor */
>>> watchdog_mon_event("pause");
>>> vm_stop(VMSTOP_WATCHDOG);
>>> + vm_status_set(VMST_WATCHDOG);
>>> break;
>>>
>>> case WDT_DEBUG:
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index cbc2532..aee9e0a 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
>>> if (ret < 0) {
>>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
>>> vm_stop(VMSTOP_PANIC);
>>> + vm_status_set(VMST_INTERROR);
>>> }
>>>
>>> env->exit_request = 0;
>>> diff --git a/migration.c b/migration.c
>>> index af3a1f2..674792f 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
>>> }
>>> state = MIG_STATE_ERROR;
>>> }
>>> + if (state == MIG_STATE_COMPLETED) {
>>> + vm_status_set(VMST_POSTMIGRATE);
>>> + }
>>> s->state = state;
>>> notifier_list_notify(&migration_state_notifiers);
>>> }
>>> diff --git a/monitor.c b/monitor.c
>>> index 67ceb46..1cb3191 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
>>> return 0;
>>> }
>>>
>>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>>>
>>> vm_stop(VMSTOP_LOADVM);
>>>
>>> - if (load_vmstate(name) == 0 && saved_vm_running) {
>>> + if (load_vmstate(name) < 0) {
>>> + vm_status_set(VMST_LOADERROR);
>>> + } else if (saved_vm_running) {
>>> vm_start();
>>> }
>>> }
>>> diff --git a/sysemu.h b/sysemu.h
>>> index d3013f5..7308ac5 100644
>>> --- a/sysemu.h
>>> +++ b/sysemu.h
>>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
>>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
>>> int qemu_loadvm_state(QEMUFile *f);
>>>
>>> +typedef enum {
>>> + VMST_NOSTATUS,
>>> + VMST_DEBUG,
>>> + VMST_INMIGRATE,
>>> + VMST_POSTMIGRATE,
>>> + VMST_INTERROR,
>>> + VMST_IOERROR,
>>> + VMST_LOADERROR,
>>> + VMST_PAUSED,
>>> + VMST_PRELAUNCH,
>>> + VMST_RUNNING,
>>> + VMST_SHUTDOWN,
>>> + VMST_WATCHDOG,
>>> + VMST_MAX,
>>> +} VMStatus;
>>
>> How are these related to the VMSTOP_*?
>>
>> Why do we need a separate enumeration?
Luiz, what about this part? For me, this was the most important
question. We already have VMSTOP_*, and every caller of vm_stop() should
change the status (most of the vm_status_set() calls come immediately
after a vm_stop() call), so it would appear logical that vm_stop(),
which already gets a reason, sets the status.
Probably we would need a few additional reasons for vm_stop(), but
keeping two separate status values for almost the same thing looks
suspicious.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-12 14:51 ` Kevin Wolf
@ 2011-07-12 15:12 ` Luiz Capitulino
2011-07-12 16:03 ` Luiz Capitulino
0 siblings, 1 reply; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 15:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, Markus Armbruster, qemu-devel, jan.kiszka, jdenemar
On Tue, 12 Jul 2011 16:51:03 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 12.07.2011 16:25, schrieb Luiz Capitulino:
> > On Tue, 12 Jul 2011 09:28:05 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>
> >>> We need to track the VM status so that QMP can report it to clients.
> >>>
> >>> This commit adds the VMStatus type and related functions. The
> >>> vm_status_set() function is used to keep track of the current
> >>> VM status.
> >>>
> >>> The current statuses are:
> >>
> >> Nitpicking about names, bear with me.
> >>
> >>> - debug: guest is running under gdb
> >>> - inmigrate: guest is paused waiting for an incoming migration
> >>
> >> incoming-migration?
> >>
> >>> - postmigrate: guest is paused following a successful migration
> >>
> >> post-migrate?
> >>
> >>> - internal-error: Fatal internal error that prevents further guest
> >>> execution
> >>> - load-state-error: guest is paused following a failed 'loadvm'
> >>
> >> Less than obvious. If you like concrete, name it loadvm-failed. If you
> >> like abstract, name it restore-vm-failed.
> >
> > Ok for your suggestions above.
> >
> >>
> >>> - io-error: the last IOP has failed and the device is configured
> >>> to pause on I/O errors
> >>> - watchdog-error: the watchdog action is configured to pause and
> >>> has been triggered
> >>
> >> Sounds like the watchdog suffered an error. watchdog-fired?
> >
> > Maybe watchdog-paused.
> >
> >>
> >>> - paused: guest has been paused via the 'stop' command
> >>
> >> stop-command?
> >
> > I prefer 'paused', it communicates better the state we're in.
> >
> >>
> >>> - prelaunch: QEMU was started with -S and guest has not started
> >>
> >> unstarted?
> >
> > Looks the same to me.
> >
> >>
> >>> - running: guest is actively running
> >>> - shutdown: guest is shut down (and -no-shutdown is in use)
> >>>
> >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >>> ---
> >>> gdbstub.c | 4 ++++
> >>> hw/ide/core.c | 1 +
> >>> hw/scsi-disk.c | 1 +
> >>> hw/virtio-blk.c | 1 +
> >>> hw/watchdog.c | 1 +
> >>> kvm-all.c | 1 +
> >>> migration.c | 3 +++
> >>> monitor.c | 5 ++++-
> >>> sysemu.h | 19 +++++++++++++++++++
> >>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> >>> 10 files changed, 72 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/gdbstub.c b/gdbstub.c
> >>> index c085a5a..61b700a 100644
> >>> --- a/gdbstub.c
> >>> +++ b/gdbstub.c
> >>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> >>> s->state = RS_SYSCALL;
> >>> #ifndef CONFIG_USER_ONLY
> >>> vm_stop(VMSTOP_DEBUG);
> >>> + vm_status_set(VMST_DEBUG);
> >>> #endif
> >>> s->state = RS_IDLE;
> >>> va_start(va, fmt);
> >>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> >>> /* when the CPU is running, we cannot do anything except stop
> >>> it when receiving a char */
> >>> vm_stop(VMSTOP_USER);
> >>> + vm_status_set(VMST_DEBUG);
> >>> } else
> >>> #endif
> >>> {
> >>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> >>> switch (event) {
> >>> case CHR_EVENT_OPENED:
> >>> vm_stop(VMSTOP_USER);
> >>> + vm_status_set(VMST_DEBUG);
> >>> gdb_has_xml = 0;
> >>> break;
> >>> default:
> >>
> >> Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
> >>
> >>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> >>> {
> >>> if (vm_running) {
> >>> vm_stop(VMSTOP_USER);
> >>> + vm_status_set(VMST_DEBUG);
> >>> }
> >>> }
> >>> #endif
> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >>> index ca17a43..bf9df41 100644
> >>> --- a/hw/ide/core.c
> >>> +++ b/hw/ide/core.c
> >>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> >>> s->bus->error_status = op;
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>> vm_stop(VMSTOP_DISKFULL);
> >>> + vm_status_set(VMST_IOERROR);
> >>> } else {
> >>> if (op & BM_STATUS_DMA_RETRY) {
> >>> dma_buf_commit(s, 0);
> >>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> >>> index a8c7372..66037fd 100644
> >>> --- a/hw/scsi-disk.c
> >>> +++ b/hw/scsi-disk.c
> >>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> >>>
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>> vm_stop(VMSTOP_DISKFULL);
> >>> + vm_status_set(VMST_IOERROR);
> >>> } else {
> >>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
> >>> scsi_req_data(&r->req, 0);
> >>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> >>> index 91e0394..bf70200 100644
> >>> --- a/hw/virtio-blk.c
> >>> +++ b/hw/virtio-blk.c
> >>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> >>> s->rq = req;
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>> vm_stop(VMSTOP_DISKFULL);
> >>> + vm_status_set(VMST_IOERROR);
> >>> } else {
> >>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> >>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> >>> diff --git a/hw/watchdog.c b/hw/watchdog.c
> >>> index 1c900a1..d130cbb 100644
> >>> --- a/hw/watchdog.c
> >>> +++ b/hw/watchdog.c
> >>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> >>> case WDT_PAUSE: /* same as 'stop' command in monitor */
> >>> watchdog_mon_event("pause");
> >>> vm_stop(VMSTOP_WATCHDOG);
> >>> + vm_status_set(VMST_WATCHDOG);
> >>> break;
> >>>
> >>> case WDT_DEBUG:
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index cbc2532..aee9e0a 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> >>> if (ret < 0) {
> >>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> >>> vm_stop(VMSTOP_PANIC);
> >>> + vm_status_set(VMST_INTERROR);
> >>> }
> >>>
> >>> env->exit_request = 0;
> >>> diff --git a/migration.c b/migration.c
> >>> index af3a1f2..674792f 100644
> >>> --- a/migration.c
> >>> +++ b/migration.c
> >>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> >>> }
> >>> state = MIG_STATE_ERROR;
> >>> }
> >>> + if (state == MIG_STATE_COMPLETED) {
> >>> + vm_status_set(VMST_POSTMIGRATE);
> >>> + }
> >>> s->state = state;
> >>> notifier_list_notify(&migration_state_notifiers);
> >>> }
> >>> diff --git a/monitor.c b/monitor.c
> >>> index 67ceb46..1cb3191 100644
> >>> --- a/monitor.c
> >>> +++ b/monitor.c
> >>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> >>> return 0;
> >>> }
> >>>
> >>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
> >>>
> >>> vm_stop(VMSTOP_LOADVM);
> >>>
> >>> - if (load_vmstate(name) == 0 && saved_vm_running) {
> >>> + if (load_vmstate(name) < 0) {
> >>> + vm_status_set(VMST_LOADERROR);
> >>> + } else if (saved_vm_running) {
> >>> vm_start();
> >>> }
> >>> }
> >>> diff --git a/sysemu.h b/sysemu.h
> >>> index d3013f5..7308ac5 100644
> >>> --- a/sysemu.h
> >>> +++ b/sysemu.h
> >>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> >>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> >>> int qemu_loadvm_state(QEMUFile *f);
> >>>
> >>> +typedef enum {
> >>> + VMST_NOSTATUS,
> >>> + VMST_DEBUG,
> >>> + VMST_INMIGRATE,
> >>> + VMST_POSTMIGRATE,
> >>> + VMST_INTERROR,
> >>> + VMST_IOERROR,
> >>> + VMST_LOADERROR,
> >>> + VMST_PAUSED,
> >>> + VMST_PRELAUNCH,
> >>> + VMST_RUNNING,
> >>> + VMST_SHUTDOWN,
> >>> + VMST_WATCHDOG,
> >>> + VMST_MAX,
> >>> +} VMStatus;
> >>
> >> How are these related to the VMSTOP_*?
> >>
> >> Why do we need a separate enumeration?
>
> Luiz, what about this part? For me, this was the most important
> question. We already have VMSTOP_*, and every caller of vm_stop() should
> change the status (most of the vm_status_set() calls come immediately
> after a vm_stop() call), so it would appear logical that vm_stop(),
> which already gets a reason, sets the status.
>
> Probably we would need a few additional reasons for vm_stop(), but
> keeping two separate status values for almost the same thing looks
> suspicious.
Well, that's how I was doing it but I had a conversation with Anthony
and he was against using vm_stop() for this, because (IIRC) they are
different things.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-12 15:12 ` Luiz Capitulino
@ 2011-07-12 16:03 ` Luiz Capitulino
2011-07-12 16:16 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 16:03 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, Markus Armbruster, qemu-devel, jan.kiszka, jdenemar
On Tue, 12 Jul 2011 12:12:31 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Tue, 12 Jul 2011 16:51:03 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
>
> > Am 12.07.2011 16:25, schrieb Luiz Capitulino:
> > > On Tue, 12 Jul 2011 09:28:05 +0200
> > > Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> > >>
> > >>> We need to track the VM status so that QMP can report it to clients.
> > >>>
> > >>> This commit adds the VMStatus type and related functions. The
> > >>> vm_status_set() function is used to keep track of the current
> > >>> VM status.
> > >>>
> > >>> The current statuses are:
> > >>
> > >> Nitpicking about names, bear with me.
> > >>
> > >>> - debug: guest is running under gdb
> > >>> - inmigrate: guest is paused waiting for an incoming migration
> > >>
> > >> incoming-migration?
> > >>
> > >>> - postmigrate: guest is paused following a successful migration
> > >>
> > >> post-migrate?
> > >>
> > >>> - internal-error: Fatal internal error that prevents further guest
> > >>> execution
> > >>> - load-state-error: guest is paused following a failed 'loadvm'
> > >>
> > >> Less than obvious. If you like concrete, name it loadvm-failed. If you
> > >> like abstract, name it restore-vm-failed.
> > >
> > > Ok for your suggestions above.
> > >
> > >>
> > >>> - io-error: the last IOP has failed and the device is configured
> > >>> to pause on I/O errors
> > >>> - watchdog-error: the watchdog action is configured to pause and
> > >>> has been triggered
> > >>
> > >> Sounds like the watchdog suffered an error. watchdog-fired?
> > >
> > > Maybe watchdog-paused.
> > >
> > >>
> > >>> - paused: guest has been paused via the 'stop' command
> > >>
> > >> stop-command?
> > >
> > > I prefer 'paused', it communicates better the state we're in.
> > >
> > >>
> > >>> - prelaunch: QEMU was started with -S and guest has not started
> > >>
> > >> unstarted?
> > >
> > > Looks the same to me.
> > >
> > >>
> > >>> - running: guest is actively running
> > >>> - shutdown: guest is shut down (and -no-shutdown is in use)
> > >>>
> > >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > >>> ---
> > >>> gdbstub.c | 4 ++++
> > >>> hw/ide/core.c | 1 +
> > >>> hw/scsi-disk.c | 1 +
> > >>> hw/virtio-blk.c | 1 +
> > >>> hw/watchdog.c | 1 +
> > >>> kvm-all.c | 1 +
> > >>> migration.c | 3 +++
> > >>> monitor.c | 5 ++++-
> > >>> sysemu.h | 19 +++++++++++++++++++
> > >>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> > >>> 10 files changed, 72 insertions(+), 1 deletions(-)
> > >>>
> > >>> diff --git a/gdbstub.c b/gdbstub.c
> > >>> index c085a5a..61b700a 100644
> > >>> --- a/gdbstub.c
> > >>> +++ b/gdbstub.c
> > >>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> > >>> s->state = RS_SYSCALL;
> > >>> #ifndef CONFIG_USER_ONLY
> > >>> vm_stop(VMSTOP_DEBUG);
> > >>> + vm_status_set(VMST_DEBUG);
> > >>> #endif
> > >>> s->state = RS_IDLE;
> > >>> va_start(va, fmt);
> > >>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> > >>> /* when the CPU is running, we cannot do anything except stop
> > >>> it when receiving a char */
> > >>> vm_stop(VMSTOP_USER);
> > >>> + vm_status_set(VMST_DEBUG);
> > >>> } else
> > >>> #endif
> > >>> {
> > >>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> > >>> switch (event) {
> > >>> case CHR_EVENT_OPENED:
> > >>> vm_stop(VMSTOP_USER);
> > >>> + vm_status_set(VMST_DEBUG);
> > >>> gdb_has_xml = 0;
> > >>> break;
> > >>> default:
> > >>
> > >> Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
> > >>
> > >>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> > >>> {
> > >>> if (vm_running) {
> > >>> vm_stop(VMSTOP_USER);
> > >>> + vm_status_set(VMST_DEBUG);
> > >>> }
> > >>> }
> > >>> #endif
> > >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> > >>> index ca17a43..bf9df41 100644
> > >>> --- a/hw/ide/core.c
> > >>> +++ b/hw/ide/core.c
> > >>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> > >>> s->bus->error_status = op;
> > >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > >>> vm_stop(VMSTOP_DISKFULL);
> > >>> + vm_status_set(VMST_IOERROR);
> > >>> } else {
> > >>> if (op & BM_STATUS_DMA_RETRY) {
> > >>> dma_buf_commit(s, 0);
> > >>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> > >>> index a8c7372..66037fd 100644
> > >>> --- a/hw/scsi-disk.c
> > >>> +++ b/hw/scsi-disk.c
> > >>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> > >>>
> > >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > >>> vm_stop(VMSTOP_DISKFULL);
> > >>> + vm_status_set(VMST_IOERROR);
> > >>> } else {
> > >>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
> > >>> scsi_req_data(&r->req, 0);
> > >>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> > >>> index 91e0394..bf70200 100644
> > >>> --- a/hw/virtio-blk.c
> > >>> +++ b/hw/virtio-blk.c
> > >>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> > >>> s->rq = req;
> > >>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > >>> vm_stop(VMSTOP_DISKFULL);
> > >>> + vm_status_set(VMST_IOERROR);
> > >>> } else {
> > >>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> > >>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> > >>> diff --git a/hw/watchdog.c b/hw/watchdog.c
> > >>> index 1c900a1..d130cbb 100644
> > >>> --- a/hw/watchdog.c
> > >>> +++ b/hw/watchdog.c
> > >>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> > >>> case WDT_PAUSE: /* same as 'stop' command in monitor */
> > >>> watchdog_mon_event("pause");
> > >>> vm_stop(VMSTOP_WATCHDOG);
> > >>> + vm_status_set(VMST_WATCHDOG);
> > >>> break;
> > >>>
> > >>> case WDT_DEBUG:
> > >>> diff --git a/kvm-all.c b/kvm-all.c
> > >>> index cbc2532..aee9e0a 100644
> > >>> --- a/kvm-all.c
> > >>> +++ b/kvm-all.c
> > >>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> > >>> if (ret < 0) {
> > >>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> > >>> vm_stop(VMSTOP_PANIC);
> > >>> + vm_status_set(VMST_INTERROR);
> > >>> }
> > >>>
> > >>> env->exit_request = 0;
> > >>> diff --git a/migration.c b/migration.c
> > >>> index af3a1f2..674792f 100644
> > >>> --- a/migration.c
> > >>> +++ b/migration.c
> > >>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> > >>> }
> > >>> state = MIG_STATE_ERROR;
> > >>> }
> > >>> + if (state == MIG_STATE_COMPLETED) {
> > >>> + vm_status_set(VMST_POSTMIGRATE);
> > >>> + }
> > >>> s->state = state;
> > >>> notifier_list_notify(&migration_state_notifiers);
> > >>> }
> > >>> diff --git a/monitor.c b/monitor.c
> > >>> index 67ceb46..1cb3191 100644
> > >>> --- a/monitor.c
> > >>> +++ b/monitor.c
> > >>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> > >>> return 0;
> > >>> }
> > >>>
> > >>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
> > >>>
> > >>> vm_stop(VMSTOP_LOADVM);
> > >>>
> > >>> - if (load_vmstate(name) == 0 && saved_vm_running) {
> > >>> + if (load_vmstate(name) < 0) {
> > >>> + vm_status_set(VMST_LOADERROR);
> > >>> + } else if (saved_vm_running) {
> > >>> vm_start();
> > >>> }
> > >>> }
> > >>> diff --git a/sysemu.h b/sysemu.h
> > >>> index d3013f5..7308ac5 100644
> > >>> --- a/sysemu.h
> > >>> +++ b/sysemu.h
> > >>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> > >>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> > >>> int qemu_loadvm_state(QEMUFile *f);
> > >>>
> > >>> +typedef enum {
> > >>> + VMST_NOSTATUS,
> > >>> + VMST_DEBUG,
> > >>> + VMST_INMIGRATE,
> > >>> + VMST_POSTMIGRATE,
> > >>> + VMST_INTERROR,
> > >>> + VMST_IOERROR,
> > >>> + VMST_LOADERROR,
> > >>> + VMST_PAUSED,
> > >>> + VMST_PRELAUNCH,
> > >>> + VMST_RUNNING,
> > >>> + VMST_SHUTDOWN,
> > >>> + VMST_WATCHDOG,
> > >>> + VMST_MAX,
> > >>> +} VMStatus;
> > >>
> > >> How are these related to the VMSTOP_*?
> > >>
> > >> Why do we need a separate enumeration?
> >
> > Luiz, what about this part? For me, this was the most important
> > question. We already have VMSTOP_*, and every caller of vm_stop() should
> > change the status (most of the vm_status_set() calls come immediately
> > after a vm_stop() call), so it would appear logical that vm_stop(),
> > which already gets a reason, sets the status.
> >
> > Probably we would need a few additional reasons for vm_stop(), but
> > keeping two separate status values for almost the same thing looks
> > suspicious.
>
> Well, that's how I was doing it but I had a conversation with Anthony
> and he was against using vm_stop() for this, because (IIRC) they are
> different things.
Let me elaborate the way I see it. The VMSTOP_* macros are stop reasons.
They say why the VM stopped, not what the VM status is. For example, "running"
is a valid vm state, but it doesn't make sense as a "stop reason".
It's possible to change vm_stop() (and vm_start()) to set the state instead,
but it's a more profound surgery than it may seem at first. For example, we
have things like vm stop notifiers, which would have to be changed to get the
vm status instead of a stop reason.
I started doing it that way and have to admit that having the vm state as
a different thing made the implementation simpler.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-12 16:03 ` Luiz Capitulino
@ 2011-07-12 16:16 ` Kevin Wolf
2011-07-12 17:59 ` Luiz Capitulino
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2011-07-12 16:16 UTC (permalink / raw)
To: Luiz Capitulino
Cc: stefanha, Markus Armbruster, qemu-devel, jan.kiszka, jdenemar
Am 12.07.2011 18:03, schrieb Luiz Capitulino:
> On Tue, 12 Jul 2011 12:12:31 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>
>> On Tue, 12 Jul 2011 16:51:03 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> Am 12.07.2011 16:25, schrieb Luiz Capitulino:
>>>> On Tue, 12 Jul 2011 09:28:05 +0200
>>>> Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>>>>
>>>>>> We need to track the VM status so that QMP can report it to clients.
>>>>>>
>>>>>> This commit adds the VMStatus type and related functions. The
>>>>>> vm_status_set() function is used to keep track of the current
>>>>>> VM status.
>>>>>>
>>>>>> The current statuses are:
>>>>>
>>>>> Nitpicking about names, bear with me.
>>>>>
>>>>>> - debug: guest is running under gdb
>>>>>> - inmigrate: guest is paused waiting for an incoming migration
>>>>>
>>>>> incoming-migration?
>>>>>
>>>>>> - postmigrate: guest is paused following a successful migration
>>>>>
>>>>> post-migrate?
>>>>>
>>>>>> - internal-error: Fatal internal error that prevents further guest
>>>>>> execution
>>>>>> - load-state-error: guest is paused following a failed 'loadvm'
>>>>>
>>>>> Less than obvious. If you like concrete, name it loadvm-failed. If you
>>>>> like abstract, name it restore-vm-failed.
>>>>
>>>> Ok for your suggestions above.
>>>>
>>>>>
>>>>>> - io-error: the last IOP has failed and the device is configured
>>>>>> to pause on I/O errors
>>>>>> - watchdog-error: the watchdog action is configured to pause and
>>>>>> has been triggered
>>>>>
>>>>> Sounds like the watchdog suffered an error. watchdog-fired?
>>>>
>>>> Maybe watchdog-paused.
>>>>
>>>>>
>>>>>> - paused: guest has been paused via the 'stop' command
>>>>>
>>>>> stop-command?
>>>>
>>>> I prefer 'paused', it communicates better the state we're in.
>>>>
>>>>>
>>>>>> - prelaunch: QEMU was started with -S and guest has not started
>>>>>
>>>>> unstarted?
>>>>
>>>> Looks the same to me.
>>>>
>>>>>
>>>>>> - running: guest is actively running
>>>>>> - shutdown: guest is shut down (and -no-shutdown is in use)
>>>>>>
>>>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>>>> ---
>>>>>> gdbstub.c | 4 ++++
>>>>>> hw/ide/core.c | 1 +
>>>>>> hw/scsi-disk.c | 1 +
>>>>>> hw/virtio-blk.c | 1 +
>>>>>> hw/watchdog.c | 1 +
>>>>>> kvm-all.c | 1 +
>>>>>> migration.c | 3 +++
>>>>>> monitor.c | 5 ++++-
>>>>>> sysemu.h | 19 +++++++++++++++++++
>>>>>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>>> 10 files changed, 72 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/gdbstub.c b/gdbstub.c
>>>>>> index c085a5a..61b700a 100644
>>>>>> --- a/gdbstub.c
>>>>>> +++ b/gdbstub.c
>>>>>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
>>>>>> s->state = RS_SYSCALL;
>>>>>> #ifndef CONFIG_USER_ONLY
>>>>>> vm_stop(VMSTOP_DEBUG);
>>>>>> + vm_status_set(VMST_DEBUG);
>>>>>> #endif
>>>>>> s->state = RS_IDLE;
>>>>>> va_start(va, fmt);
>>>>>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
>>>>>> /* when the CPU is running, we cannot do anything except stop
>>>>>> it when receiving a char */
>>>>>> vm_stop(VMSTOP_USER);
>>>>>> + vm_status_set(VMST_DEBUG);
>>>>>> } else
>>>>>> #endif
>>>>>> {
>>>>>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
>>>>>> switch (event) {
>>>>>> case CHR_EVENT_OPENED:
>>>>>> vm_stop(VMSTOP_USER);
>>>>>> + vm_status_set(VMST_DEBUG);
>>>>>> gdb_has_xml = 0;
>>>>>> break;
>>>>>> default:
>>>>>
>>>>> Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
>>>>>
>>>>>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
>>>>>> {
>>>>>> if (vm_running) {
>>>>>> vm_stop(VMSTOP_USER);
>>>>>> + vm_status_set(VMST_DEBUG);
>>>>>> }
>>>>>> }
>>>>>> #endif
>>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>>>> index ca17a43..bf9df41 100644
>>>>>> --- a/hw/ide/core.c
>>>>>> +++ b/hw/ide/core.c
>>>>>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>>>>>> s->bus->error_status = op;
>>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>>>>> vm_stop(VMSTOP_DISKFULL);
>>>>>> + vm_status_set(VMST_IOERROR);
>>>>>> } else {
>>>>>> if (op & BM_STATUS_DMA_RETRY) {
>>>>>> dma_buf_commit(s, 0);
>>>>>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
>>>>>> index a8c7372..66037fd 100644
>>>>>> --- a/hw/scsi-disk.c
>>>>>> +++ b/hw/scsi-disk.c
>>>>>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
>>>>>>
>>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>>>>> vm_stop(VMSTOP_DISKFULL);
>>>>>> + vm_status_set(VMST_IOERROR);
>>>>>> } else {
>>>>>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
>>>>>> scsi_req_data(&r->req, 0);
>>>>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>>>>> index 91e0394..bf70200 100644
>>>>>> --- a/hw/virtio-blk.c
>>>>>> +++ b/hw/virtio-blk.c
>>>>>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>>>>>> s->rq = req;
>>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
>>>>>> vm_stop(VMSTOP_DISKFULL);
>>>>>> + vm_status_set(VMST_IOERROR);
>>>>>> } else {
>>>>>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
>>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
>>>>>> diff --git a/hw/watchdog.c b/hw/watchdog.c
>>>>>> index 1c900a1..d130cbb 100644
>>>>>> --- a/hw/watchdog.c
>>>>>> +++ b/hw/watchdog.c
>>>>>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
>>>>>> case WDT_PAUSE: /* same as 'stop' command in monitor */
>>>>>> watchdog_mon_event("pause");
>>>>>> vm_stop(VMSTOP_WATCHDOG);
>>>>>> + vm_status_set(VMST_WATCHDOG);
>>>>>> break;
>>>>>>
>>>>>> case WDT_DEBUG:
>>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>>> index cbc2532..aee9e0a 100644
>>>>>> --- a/kvm-all.c
>>>>>> +++ b/kvm-all.c
>>>>>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
>>>>>> if (ret < 0) {
>>>>>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
>>>>>> vm_stop(VMSTOP_PANIC);
>>>>>> + vm_status_set(VMST_INTERROR);
>>>>>> }
>>>>>>
>>>>>> env->exit_request = 0;
>>>>>> diff --git a/migration.c b/migration.c
>>>>>> index af3a1f2..674792f 100644
>>>>>> --- a/migration.c
>>>>>> +++ b/migration.c
>>>>>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
>>>>>> }
>>>>>> state = MIG_STATE_ERROR;
>>>>>> }
>>>>>> + if (state == MIG_STATE_COMPLETED) {
>>>>>> + vm_status_set(VMST_POSTMIGRATE);
>>>>>> + }
>>>>>> s->state = state;
>>>>>> notifier_list_notify(&migration_state_notifiers);
>>>>>> }
>>>>>> diff --git a/monitor.c b/monitor.c
>>>>>> index 67ceb46..1cb3191 100644
>>>>>> --- a/monitor.c
>>>>>> +++ b/monitor.c
>>>>>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>>>>>>
>>>>>> vm_stop(VMSTOP_LOADVM);
>>>>>>
>>>>>> - if (load_vmstate(name) == 0 && saved_vm_running) {
>>>>>> + if (load_vmstate(name) < 0) {
>>>>>> + vm_status_set(VMST_LOADERROR);
>>>>>> + } else if (saved_vm_running) {
>>>>>> vm_start();
>>>>>> }
>>>>>> }
>>>>>> diff --git a/sysemu.h b/sysemu.h
>>>>>> index d3013f5..7308ac5 100644
>>>>>> --- a/sysemu.h
>>>>>> +++ b/sysemu.h
>>>>>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
>>>>>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
>>>>>> int qemu_loadvm_state(QEMUFile *f);
>>>>>>
>>>>>> +typedef enum {
>>>>>> + VMST_NOSTATUS,
>>>>>> + VMST_DEBUG,
>>>>>> + VMST_INMIGRATE,
>>>>>> + VMST_POSTMIGRATE,
>>>>>> + VMST_INTERROR,
>>>>>> + VMST_IOERROR,
>>>>>> + VMST_LOADERROR,
>>>>>> + VMST_PAUSED,
>>>>>> + VMST_PRELAUNCH,
>>>>>> + VMST_RUNNING,
>>>>>> + VMST_SHUTDOWN,
>>>>>> + VMST_WATCHDOG,
>>>>>> + VMST_MAX,
>>>>>> +} VMStatus;
>>>>>
>>>>> How are these related to the VMSTOP_*?
>>>>>
>>>>> Why do we need a separate enumeration?
>>>
>>> Luiz, what about this part? For me, this was the most important
>>> question. We already have VMSTOP_*, and every caller of vm_stop() should
>>> change the status (most of the vm_status_set() calls come immediately
>>> after a vm_stop() call), so it would appear logical that vm_stop(),
>>> which already gets a reason, sets the status.
>>>
>>> Probably we would need a few additional reasons for vm_stop(), but
>>> keeping two separate status values for almost the same thing looks
>>> suspicious.
>>
>> Well, that's how I was doing it but I had a conversation with Anthony
>> and he was against using vm_stop() for this, because (IIRC) they are
>> different things.
>
> Let me elaborate the way I see it. The VMSTOP_* macros are stop reasons.
> They say why the VM stopped, not what the VM status is. For example, "running"
> is a valid vm state, but it doesn't make sense as a "stop reason".
>
> It's possible to change vm_stop() (and vm_start()) to set the state instead,
> but it's a more profound surgery than it may seem at first. For example, we
> have things like vm stop notifiers, which would have to be changed to get the
> vm status instead of a stop reason.
>
> I started doing it that way and have to admit that having the vm state as
> a different thing made the implementation simpler.
Maybe we can have vm_stop() take two arguments at least, so that you
can't forget updating the status when you call vm_stop()? Or are there
cases of vm_stop() that shouldn't change the status?
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type
2011-07-12 16:16 ` Kevin Wolf
@ 2011-07-12 17:59 ` Luiz Capitulino
0 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 17:59 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, Markus Armbruster, qemu-devel, jan.kiszka, jdenemar
On Tue, 12 Jul 2011 18:16:26 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 12.07.2011 18:03, schrieb Luiz Capitulino:
> > On Tue, 12 Jul 2011 12:12:31 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> >
> >> On Tue, 12 Jul 2011 16:51:03 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> Am 12.07.2011 16:25, schrieb Luiz Capitulino:
> >>>> On Tue, 12 Jul 2011 09:28:05 +0200
> >>>> Markus Armbruster <armbru@redhat.com> wrote:
> >>>>
> >>>>> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>>>>
> >>>>>> We need to track the VM status so that QMP can report it to clients.
> >>>>>>
> >>>>>> This commit adds the VMStatus type and related functions. The
> >>>>>> vm_status_set() function is used to keep track of the current
> >>>>>> VM status.
> >>>>>>
> >>>>>> The current statuses are:
> >>>>>
> >>>>> Nitpicking about names, bear with me.
> >>>>>
> >>>>>> - debug: guest is running under gdb
> >>>>>> - inmigrate: guest is paused waiting for an incoming migration
> >>>>>
> >>>>> incoming-migration?
> >>>>>
> >>>>>> - postmigrate: guest is paused following a successful migration
> >>>>>
> >>>>> post-migrate?
> >>>>>
> >>>>>> - internal-error: Fatal internal error that prevents further guest
> >>>>>> execution
> >>>>>> - load-state-error: guest is paused following a failed 'loadvm'
> >>>>>
> >>>>> Less than obvious. If you like concrete, name it loadvm-failed. If you
> >>>>> like abstract, name it restore-vm-failed.
> >>>>
> >>>> Ok for your suggestions above.
> >>>>
> >>>>>
> >>>>>> - io-error: the last IOP has failed and the device is configured
> >>>>>> to pause on I/O errors
> >>>>>> - watchdog-error: the watchdog action is configured to pause and
> >>>>>> has been triggered
> >>>>>
> >>>>> Sounds like the watchdog suffered an error. watchdog-fired?
> >>>>
> >>>> Maybe watchdog-paused.
> >>>>
> >>>>>
> >>>>>> - paused: guest has been paused via the 'stop' command
> >>>>>
> >>>>> stop-command?
> >>>>
> >>>> I prefer 'paused', it communicates better the state we're in.
> >>>>
> >>>>>
> >>>>>> - prelaunch: QEMU was started with -S and guest has not started
> >>>>>
> >>>>> unstarted?
> >>>>
> >>>> Looks the same to me.
> >>>>
> >>>>>
> >>>>>> - running: guest is actively running
> >>>>>> - shutdown: guest is shut down (and -no-shutdown is in use)
> >>>>>>
> >>>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >>>>>> ---
> >>>>>> gdbstub.c | 4 ++++
> >>>>>> hw/ide/core.c | 1 +
> >>>>>> hw/scsi-disk.c | 1 +
> >>>>>> hw/virtio-blk.c | 1 +
> >>>>>> hw/watchdog.c | 1 +
> >>>>>> kvm-all.c | 1 +
> >>>>>> migration.c | 3 +++
> >>>>>> monitor.c | 5 ++++-
> >>>>>> sysemu.h | 19 +++++++++++++++++++
> >>>>>> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> >>>>>> 10 files changed, 72 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>> diff --git a/gdbstub.c b/gdbstub.c
> >>>>>> index c085a5a..61b700a 100644
> >>>>>> --- a/gdbstub.c
> >>>>>> +++ b/gdbstub.c
> >>>>>> @@ -2358,6 +2358,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
> >>>>>> s->state = RS_SYSCALL;
> >>>>>> #ifndef CONFIG_USER_ONLY
> >>>>>> vm_stop(VMSTOP_DEBUG);
> >>>>>> + vm_status_set(VMST_DEBUG);
> >>>>>> #endif
> >>>>>> s->state = RS_IDLE;
> >>>>>> va_start(va, fmt);
> >>>>>> @@ -2432,6 +2433,7 @@ static void gdb_read_byte(GDBState *s, int ch)
> >>>>>> /* when the CPU is running, we cannot do anything except stop
> >>>>>> it when receiving a char */
> >>>>>> vm_stop(VMSTOP_USER);
> >>>>>> + vm_status_set(VMST_DEBUG);
> >>>>>> } else
> >>>>>> #endif
> >>>>>> {
> >>>>>> @@ -2694,6 +2696,7 @@ static void gdb_chr_event(void *opaque, int event)
> >>>>>> switch (event) {
> >>>>>> case CHR_EVENT_OPENED:
> >>>>>> vm_stop(VMSTOP_USER);
> >>>>>> + vm_status_set(VMST_DEBUG);
> >>>>>> gdb_has_xml = 0;
> >>>>>> break;
> >>>>>> default:
> >>>>>
> >>>>> Previous hunk has VMST_DEBUG with VMST_DEBUG. Odd.
> >>>>>
> >>>>>> @@ -2735,6 +2738,7 @@ static void gdb_sigterm_handler(int signal)
> >>>>>> {
> >>>>>> if (vm_running) {
> >>>>>> vm_stop(VMSTOP_USER);
> >>>>>> + vm_status_set(VMST_DEBUG);
> >>>>>> }
> >>>>>> }
> >>>>>> #endif
> >>>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >>>>>> index ca17a43..bf9df41 100644
> >>>>>> --- a/hw/ide/core.c
> >>>>>> +++ b/hw/ide/core.c
> >>>>>> @@ -523,6 +523,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> >>>>>> s->bus->error_status = op;
> >>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>>>>> vm_stop(VMSTOP_DISKFULL);
> >>>>>> + vm_status_set(VMST_IOERROR);
> >>>>>> } else {
> >>>>>> if (op & BM_STATUS_DMA_RETRY) {
> >>>>>> dma_buf_commit(s, 0);
> >>>>>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> >>>>>> index a8c7372..66037fd 100644
> >>>>>> --- a/hw/scsi-disk.c
> >>>>>> +++ b/hw/scsi-disk.c
> >>>>>> @@ -216,6 +216,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
> >>>>>>
> >>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>>>>> vm_stop(VMSTOP_DISKFULL);
> >>>>>> + vm_status_set(VMST_IOERROR);
> >>>>>> } else {
> >>>>>> if (type == SCSI_REQ_STATUS_RETRY_READ) {
> >>>>>> scsi_req_data(&r->req, 0);
> >>>>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> >>>>>> index 91e0394..bf70200 100644
> >>>>>> --- a/hw/virtio-blk.c
> >>>>>> +++ b/hw/virtio-blk.c
> >>>>>> @@ -79,6 +79,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> >>>>>> s->rq = req;
> >>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> >>>>>> vm_stop(VMSTOP_DISKFULL);
> >>>>>> + vm_status_set(VMST_IOERROR);
> >>>>>> } else {
> >>>>>> virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> >>>>>> bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> >>>>>> diff --git a/hw/watchdog.c b/hw/watchdog.c
> >>>>>> index 1c900a1..d130cbb 100644
> >>>>>> --- a/hw/watchdog.c
> >>>>>> +++ b/hw/watchdog.c
> >>>>>> @@ -133,6 +133,7 @@ void watchdog_perform_action(void)
> >>>>>> case WDT_PAUSE: /* same as 'stop' command in monitor */
> >>>>>> watchdog_mon_event("pause");
> >>>>>> vm_stop(VMSTOP_WATCHDOG);
> >>>>>> + vm_status_set(VMST_WATCHDOG);
> >>>>>> break;
> >>>>>>
> >>>>>> case WDT_DEBUG:
> >>>>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>>>> index cbc2532..aee9e0a 100644
> >>>>>> --- a/kvm-all.c
> >>>>>> +++ b/kvm-all.c
> >>>>>> @@ -1015,6 +1015,7 @@ int kvm_cpu_exec(CPUState *env)
> >>>>>> if (ret < 0) {
> >>>>>> cpu_dump_state(env, stderr, fprintf, CPU_DUMP_CODE);
> >>>>>> vm_stop(VMSTOP_PANIC);
> >>>>>> + vm_status_set(VMST_INTERROR);
> >>>>>> }
> >>>>>>
> >>>>>> env->exit_request = 0;
> >>>>>> diff --git a/migration.c b/migration.c
> >>>>>> index af3a1f2..674792f 100644
> >>>>>> --- a/migration.c
> >>>>>> +++ b/migration.c
> >>>>>> @@ -394,6 +394,9 @@ void migrate_fd_put_ready(void *opaque)
> >>>>>> }
> >>>>>> state = MIG_STATE_ERROR;
> >>>>>> }
> >>>>>> + if (state == MIG_STATE_COMPLETED) {
> >>>>>> + vm_status_set(VMST_POSTMIGRATE);
> >>>>>> + }
> >>>>>> s->state = state;
> >>>>>> notifier_list_notify(&migration_state_notifiers);
> >>>>>> }
> >>>>>> diff --git a/monitor.c b/monitor.c
> >>>>>> index 67ceb46..1cb3191 100644
> >>>>>> --- a/monitor.c
> >>>>>> +++ b/monitor.c
> >>>>>> @@ -1258,6 +1258,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_status_set(VMST_PAUSED);
> >>>>>> return 0;
> >>>>>> }
> >>>>>>
> >>>>>> @@ -2782,7 +2783,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
> >>>>>>
> >>>>>> vm_stop(VMSTOP_LOADVM);
> >>>>>>
> >>>>>> - if (load_vmstate(name) == 0 && saved_vm_running) {
> >>>>>> + if (load_vmstate(name) < 0) {
> >>>>>> + vm_status_set(VMST_LOADERROR);
> >>>>>> + } else if (saved_vm_running) {
> >>>>>> vm_start();
> >>>>>> }
> >>>>>> }
> >>>>>> diff --git a/sysemu.h b/sysemu.h
> >>>>>> index d3013f5..7308ac5 100644
> >>>>>> --- a/sysemu.h
> >>>>>> +++ b/sysemu.h
> >>>>>> @@ -77,6 +77,25 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f);
> >>>>>> void qemu_savevm_state_cancel(Monitor *mon, QEMUFile *f);
> >>>>>> int qemu_loadvm_state(QEMUFile *f);
> >>>>>>
> >>>>>> +typedef enum {
> >>>>>> + VMST_NOSTATUS,
> >>>>>> + VMST_DEBUG,
> >>>>>> + VMST_INMIGRATE,
> >>>>>> + VMST_POSTMIGRATE,
> >>>>>> + VMST_INTERROR,
> >>>>>> + VMST_IOERROR,
> >>>>>> + VMST_LOADERROR,
> >>>>>> + VMST_PAUSED,
> >>>>>> + VMST_PRELAUNCH,
> >>>>>> + VMST_RUNNING,
> >>>>>> + VMST_SHUTDOWN,
> >>>>>> + VMST_WATCHDOG,
> >>>>>> + VMST_MAX,
> >>>>>> +} VMStatus;
> >>>>>
> >>>>> How are these related to the VMSTOP_*?
> >>>>>
> >>>>> Why do we need a separate enumeration?
> >>>
> >>> Luiz, what about this part? For me, this was the most important
> >>> question. We already have VMSTOP_*, and every caller of vm_stop() should
> >>> change the status (most of the vm_status_set() calls come immediately
> >>> after a vm_stop() call), so it would appear logical that vm_stop(),
> >>> which already gets a reason, sets the status.
> >>>
> >>> Probably we would need a few additional reasons for vm_stop(), but
> >>> keeping two separate status values for almost the same thing looks
> >>> suspicious.
> >>
> >> Well, that's how I was doing it but I had a conversation with Anthony
> >> and he was against using vm_stop() for this, because (IIRC) they are
> >> different things.
> >
> > Let me elaborate the way I see it. The VMSTOP_* macros are stop reasons.
> > They say why the VM stopped, not what the VM status is. For example, "running"
> > is a valid vm state, but it doesn't make sense as a "stop reason".
> >
> > It's possible to change vm_stop() (and vm_start()) to set the state instead,
> > but it's a more profound surgery than it may seem at first. For example, we
> > have things like vm stop notifiers, which would have to be changed to get the
> > vm status instead of a stop reason.
> >
> > I started doing it that way and have to admit that having the vm state as
> > a different thing made the implementation simpler.
>
> Maybe we can have vm_stop() take two arguments at least, so that you
> can't forget updating the status when you call vm_stop()? Or are there
> cases of vm_stop() that shouldn't change the status?
Today we don't set when saving the vm_state, not sure we should.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 2/8] QMP: query-status: Introduce 'status' key
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status Luiz Capitulino
` (6 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
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 | 17 ++++++++++++++++-
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/monitor.c b/monitor.c
index 1cb3191..00ebc8b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2584,8 +2584,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);
+ *ret_data = qobject_from_jsonf("{ 'running': %i, 'singlestep': %i, 'status': %s }", vm_running, singlestep, vm_status_get_name());
}
static qemu_acl *find_acl(Monitor *mon, const char *name)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 92c5c3a..6b8eb0a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1486,11 +1486,26 @@ 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
+ "postmigrate" - guest is paused following a successful 'migrate'
+ "internal-error" - An internal error that prevents further guest
+ execution has occurred
+ "load-state-error" - guest is paused following a failed 'loadvm'
+ "io-error" - the last IOP has failed and the device is configured
+ to pause on I/O errors
+ "watchdog-error" - the watchdog action is configured to pause and
+ "paused" - guest has been paused via the 'stop' command
+ "prelaunch" - QEMU was started with -S and guest has not started
+ "running" - guest is actively running
+ "shutdown" - guest is shut down (and -no-shutdown is in use)
+ has been triggered
Example:
-> { "execute": "query-status" }
-<- { "return": { "running": true, "singlestep": false } }
+<- { "return": { "running": true, "singlestep": false, "status": "running" } }
EQMP
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 2/8] QMP: query-status: Introduce 'status' key Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-12 7:45 ` Markus Armbruster
2011-07-12 14:25 ` Kevin Wolf
2011-07-05 18:17 ` [Qemu-devel] [PATCH 4/8] ide: Support " Luiz Capitulino
` (5 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
This commit adds support to the BlockDriverState type to keep track
of the last I/O status. That is, at every I/O operation we update
a status field in the BlockDriverState instance. Valid statuses are:
OK, FAILED and ENOSPC.
ENOSPC is distinguished from FAILED because an management application
can use it to implement thin-provisioning.
This feature has to be explicit enabled by buses/devices supporting it.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
block.c | 18 ++++++++++++++++++
block.h | 7 +++++++
block_int.h | 2 ++
3 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/block.c b/block.c
index 24a25d5..cc0a34e 100644
--- a/block.c
+++ b/block.c
@@ -195,6 +195,7 @@ BlockDriverState *bdrv_new(const char *device_name)
if (device_name[0] != '\0') {
QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
}
+ bs->iostatus_enabled = false;
return bs;
}
@@ -2876,6 +2877,23 @@ int bdrv_in_use(BlockDriverState *bs)
return bs->in_use;
}
+void bdrv_enable_iostatus(BlockDriverState *bs)
+{
+ bs->iostatus_enabled = true;
+}
+
+void bdrv_iostatus_update(BlockDriverState *bs, int error)
+{
+ error = abs(error);
+
+ if (!error) {
+ bs->iostatus = BDRV_IOS_OK;
+ } else {
+ bs->iostatus = (error == ENOSPC) ? BDRV_IOS_ENOSPC :
+ BDRV_IOS_FAILED;
+ }
+}
+
int bdrv_img_create(const char *filename, const char *fmt,
const char *base_filename, const char *base_fmt,
char *options, uint64_t img_size, int flags)
diff --git a/block.h b/block.h
index 859d1d9..0dca1bb 100644
--- a/block.h
+++ b/block.h
@@ -50,6 +50,13 @@ typedef enum {
BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
} BlockMonEventAction;
+typedef enum {
+ BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
+} BlockIOStatus;
+
+void bdrv_iostatus_update(BlockDriverState *bs, int error);
+void bdrv_enable_iostatus(BlockDriverState *bs);
+void bdrv_enable_io_status(BlockDriverState *bs);
void bdrv_mon_event(const BlockDriverState *bdrv,
BlockMonEventAction action, int is_read);
void bdrv_info_print(Monitor *mon, const QObject *data);
diff --git a/block_int.h b/block_int.h
index 1e265d2..09f038d 100644
--- a/block_int.h
+++ b/block_int.h
@@ -195,6 +195,8 @@ struct BlockDriverState {
drivers. They are not used by the block driver */
int cyls, heads, secs, translation;
BlockErrorAction on_read_error, on_write_error;
+ bool iostatus_enabled;
+ BlockIOStatus iostatus;
char device_name[32];
unsigned long *dirty_bitmap;
int64_t dirty_count;
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-05 18:17 ` [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status Luiz Capitulino
@ 2011-07-12 7:45 ` Markus Armbruster
2011-07-12 8:33 ` Kevin Wolf
2011-07-12 14:25 ` Kevin Wolf
1 sibling, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2011-07-12 7:45 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, jan.kiszka, jdenemar, qemu-devel, stefanha
Luiz Capitulino <lcapitulino@redhat.com> writes:
> This commit adds support to the BlockDriverState type to keep track
> of the last I/O status. That is, at every I/O operation we update
> a status field in the BlockDriverState instance. Valid statuses are:
> OK, FAILED and ENOSPC.
>
> ENOSPC is distinguished from FAILED because an management application
> can use it to implement thin-provisioning.
>
> This feature has to be explicit enabled by buses/devices supporting it.
buses?
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> block.c | 18 ++++++++++++++++++
> block.h | 7 +++++++
> block_int.h | 2 ++
> 3 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/block.c b/block.c
> index 24a25d5..cc0a34e 100644
> --- a/block.c
> +++ b/block.c
> @@ -195,6 +195,7 @@ BlockDriverState *bdrv_new(const char *device_name)
> if (device_name[0] != '\0') {
> QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> }
> + bs->iostatus_enabled = false;
> return bs;
> }
>
> @@ -2876,6 +2877,23 @@ int bdrv_in_use(BlockDriverState *bs)
> return bs->in_use;
> }
>
> +void bdrv_enable_iostatus(BlockDriverState *bs)
> +{
> + bs->iostatus_enabled = true;
> +}
> +
> +void bdrv_iostatus_update(BlockDriverState *bs, int error)
> +{
> + error = abs(error);
> +
> + if (!error) {
> + bs->iostatus = BDRV_IOS_OK;
> + } else {
> + bs->iostatus = (error == ENOSPC) ? BDRV_IOS_ENOSPC :
> + BDRV_IOS_FAILED;
> + }
> +}
> +
> int bdrv_img_create(const char *filename, const char *fmt,
> const char *base_filename, const char *base_fmt,
> char *options, uint64_t img_size, int flags)
> diff --git a/block.h b/block.h
> index 859d1d9..0dca1bb 100644
> --- a/block.h
> +++ b/block.h
> @@ -50,6 +50,13 @@ typedef enum {
> BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> } BlockMonEventAction;
>
> +typedef enum {
> + BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
> +} BlockIOStatus;
> +
> +void bdrv_iostatus_update(BlockDriverState *bs, int error);
> +void bdrv_enable_iostatus(BlockDriverState *bs);
> +void bdrv_enable_io_status(BlockDriverState *bs);
> void bdrv_mon_event(const BlockDriverState *bdrv,
> BlockMonEventAction action, int is_read);
> void bdrv_info_print(Monitor *mon, const QObject *data);
> diff --git a/block_int.h b/block_int.h
> index 1e265d2..09f038d 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -195,6 +195,8 @@ struct BlockDriverState {
> drivers. They are not used by the block driver */
> int cyls, heads, secs, translation;
> BlockErrorAction on_read_error, on_write_error;
> + bool iostatus_enabled;
> + BlockIOStatus iostatus;
> char device_name[32];
> unsigned long *dirty_bitmap;
> int64_t dirty_count;
Okay, let's see what we got here.
The block layer merely holds I/O status, device models set it.
Device I/O status is not migrated. Why?
bdrv_new() creates the BDS with I/O status tracking disabled. Devices
that do tracking enable it in their qdev init method. If a device gets
hot unplugged, tracking remains enabled. If the BDS then gets reused
with a device that doesn't do tracking, I/O status becomes incorrect.
Can't happen right now, because we automatically delete the BDS on hot
unplug, but it's a trap. Suggest to disable tracking in bdrv_detach().
Actually, this is a symptom of the midlayer disease. I suspect things
would be simpler if we hold the status in its rightful owner, the device
model. Need a getter for it. I'm working on a patch series that moves
misplaced state out of the block layer into device models and block
drivers, and a I/O status getter will fit in easily there.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-12 7:45 ` Markus Armbruster
@ 2011-07-12 8:33 ` Kevin Wolf
2011-07-12 9:12 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2011-07-12 8:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: stefanha, jan.kiszka, jdenemar, qemu-devel, Luiz Capitulino
Am 12.07.2011 09:45, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> This commit adds support to the BlockDriverState type to keep track
>> of the last I/O status. That is, at every I/O operation we update
>> a status field in the BlockDriverState instance. Valid statuses are:
>> OK, FAILED and ENOSPC.
>>
>> ENOSPC is distinguished from FAILED because an management application
>> can use it to implement thin-provisioning.
>>
>> This feature has to be explicit enabled by buses/devices supporting it.
>
> buses?
>
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>> block.c | 18 ++++++++++++++++++
>> block.h | 7 +++++++
>> block_int.h | 2 ++
>> 3 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 24a25d5..cc0a34e 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -195,6 +195,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>> if (device_name[0] != '\0') {
>> QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>> }
>> + bs->iostatus_enabled = false;
>> return bs;
>> }
>>
>> @@ -2876,6 +2877,23 @@ int bdrv_in_use(BlockDriverState *bs)
>> return bs->in_use;
>> }
>>
>> +void bdrv_enable_iostatus(BlockDriverState *bs)
>> +{
>> + bs->iostatus_enabled = true;
>> +}
>> +
>> +void bdrv_iostatus_update(BlockDriverState *bs, int error)
>> +{
>> + error = abs(error);
>> +
>> + if (!error) {
>> + bs->iostatus = BDRV_IOS_OK;
>> + } else {
>> + bs->iostatus = (error == ENOSPC) ? BDRV_IOS_ENOSPC :
>> + BDRV_IOS_FAILED;
>> + }
>> +}
>> +
>> int bdrv_img_create(const char *filename, const char *fmt,
>> const char *base_filename, const char *base_fmt,
>> char *options, uint64_t img_size, int flags)
>> diff --git a/block.h b/block.h
>> index 859d1d9..0dca1bb 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -50,6 +50,13 @@ typedef enum {
>> BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>> } BlockMonEventAction;
>>
>> +typedef enum {
>> + BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
>> +} BlockIOStatus;
>> +
>> +void bdrv_iostatus_update(BlockDriverState *bs, int error);
>> +void bdrv_enable_iostatus(BlockDriverState *bs);
>> +void bdrv_enable_io_status(BlockDriverState *bs);
>> void bdrv_mon_event(const BlockDriverState *bdrv,
>> BlockMonEventAction action, int is_read);
>> void bdrv_info_print(Monitor *mon, const QObject *data);
>> diff --git a/block_int.h b/block_int.h
>> index 1e265d2..09f038d 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -195,6 +195,8 @@ struct BlockDriverState {
>> drivers. They are not used by the block driver */
>> int cyls, heads, secs, translation;
>> BlockErrorAction on_read_error, on_write_error;
>> + bool iostatus_enabled;
>> + BlockIOStatus iostatus;
>> char device_name[32];
>> unsigned long *dirty_bitmap;
>> int64_t dirty_count;
>
> Okay, let's see what we got here.
>
> The block layer merely holds I/O status, device models set it.
>
> Device I/O status is not migrated. Why?
>
> bdrv_new() creates the BDS with I/O status tracking disabled. Devices
> that do tracking enable it in their qdev init method. If a device gets
> hot unplugged, tracking remains enabled. If the BDS then gets reused
> with a device that doesn't do tracking, I/O status becomes incorrect.
> Can't happen right now, because we automatically delete the BDS on hot
> unplug, but it's a trap. Suggest to disable tracking in bdrv_detach().
>
> Actually, this is a symptom of the midlayer disease. I suspect things
> would be simpler if we hold the status in its rightful owner, the device
> model. Need a getter for it. I'm working on a patch series that moves
> misplaced state out of the block layer into device models and block
> drivers, and a I/O status getter will fit in easily there.
This is host state, so the device is not the rightful owner. Devices
should not even be involved with enabling it.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-12 8:33 ` Kevin Wolf
@ 2011-07-12 9:12 ` Markus Armbruster
2011-07-12 14:38 ` Luiz Capitulino
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2011-07-12 9:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, jdenemar, Luiz Capitulino, qemu-devel, jan.kiszka
Kevin Wolf <kwolf@redhat.com> writes:
> Am 12.07.2011 09:45, schrieb Markus Armbruster:
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>
>>> This commit adds support to the BlockDriverState type to keep track
>>> of the last I/O status. That is, at every I/O operation we update
>>> a status field in the BlockDriverState instance. Valid statuses are:
>>> OK, FAILED and ENOSPC.
>>>
>>> ENOSPC is distinguished from FAILED because an management application
>>> can use it to implement thin-provisioning.
>>>
>>> This feature has to be explicit enabled by buses/devices supporting it.
>>
>> buses?
>>
>>>
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>> block.c | 18 ++++++++++++++++++
>>> block.h | 7 +++++++
>>> block_int.h | 2 ++
>>> 3 files changed, 27 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 24a25d5..cc0a34e 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -195,6 +195,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>>> if (device_name[0] != '\0') {
>>> QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>>> }
>>> + bs->iostatus_enabled = false;
>>> return bs;
>>> }
>>>
>>> @@ -2876,6 +2877,23 @@ int bdrv_in_use(BlockDriverState *bs)
>>> return bs->in_use;
>>> }
>>>
>>> +void bdrv_enable_iostatus(BlockDriverState *bs)
>>> +{
>>> + bs->iostatus_enabled = true;
>>> +}
>>> +
>>> +void bdrv_iostatus_update(BlockDriverState *bs, int error)
>>> +{
>>> + error = abs(error);
>>> +
>>> + if (!error) {
>>> + bs->iostatus = BDRV_IOS_OK;
>>> + } else {
>>> + bs->iostatus = (error == ENOSPC) ? BDRV_IOS_ENOSPC :
>>> + BDRV_IOS_FAILED;
>>> + }
>>> +}
>>> +
>>> int bdrv_img_create(const char *filename, const char *fmt,
>>> const char *base_filename, const char *base_fmt,
>>> char *options, uint64_t img_size, int flags)
>>> diff --git a/block.h b/block.h
>>> index 859d1d9..0dca1bb 100644
>>> --- a/block.h
>>> +++ b/block.h
>>> @@ -50,6 +50,13 @@ typedef enum {
>>> BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>>> } BlockMonEventAction;
>>>
>>> +typedef enum {
>>> + BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
>>> +} BlockIOStatus;
>>> +
>>> +void bdrv_iostatus_update(BlockDriverState *bs, int error);
>>> +void bdrv_enable_iostatus(BlockDriverState *bs);
>>> +void bdrv_enable_io_status(BlockDriverState *bs);
>>> void bdrv_mon_event(const BlockDriverState *bdrv,
>>> BlockMonEventAction action, int is_read);
>>> void bdrv_info_print(Monitor *mon, const QObject *data);
>>> diff --git a/block_int.h b/block_int.h
>>> index 1e265d2..09f038d 100644
>>> --- a/block_int.h
>>> +++ b/block_int.h
>>> @@ -195,6 +195,8 @@ struct BlockDriverState {
>>> drivers. They are not used by the block driver */
>>> int cyls, heads, secs, translation;
>>> BlockErrorAction on_read_error, on_write_error;
>>> + bool iostatus_enabled;
>>> + BlockIOStatus iostatus;
>>> char device_name[32];
>>> unsigned long *dirty_bitmap;
>>> int64_t dirty_count;
>>
>> Okay, let's see what we got here.
>>
>> The block layer merely holds I/O status, device models set it.
>>
>> Device I/O status is not migrated. Why?
>>
>> bdrv_new() creates the BDS with I/O status tracking disabled. Devices
>> that do tracking enable it in their qdev init method. If a device gets
>> hot unplugged, tracking remains enabled. If the BDS then gets reused
>> with a device that doesn't do tracking, I/O status becomes incorrect.
>> Can't happen right now, because we automatically delete the BDS on hot
>> unplug, but it's a trap. Suggest to disable tracking in bdrv_detach().
>>
>> Actually, this is a symptom of the midlayer disease. I suspect things
>> would be simpler if we hold the status in its rightful owner, the device
>> model. Need a getter for it. I'm working on a patch series that moves
>> misplaced state out of the block layer into device models and block
>> drivers, and a I/O status getter will fit in easily there.
>
> This is host state, so the device is not the rightful owner. Devices
> should not even be involved with enabling it.
They are because they do the tracking, and thus the tracking only works
for device models that do it. Could it be done entirely within the
block layer?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-12 9:12 ` Markus Armbruster
@ 2011-07-12 14:38 ` Luiz Capitulino
0 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 14:38 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, stefanha, jdenemar, qemu-devel, jan.kiszka
On Tue, 12 Jul 2011 11:12:04 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 12.07.2011 09:45, schrieb Markus Armbruster:
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>
> >>> This commit adds support to the BlockDriverState type to keep track
> >>> of the last I/O status. That is, at every I/O operation we update
> >>> a status field in the BlockDriverState instance. Valid statuses are:
> >>> OK, FAILED and ENOSPC.
> >>>
> >>> ENOSPC is distinguished from FAILED because an management application
> >>> can use it to implement thin-provisioning.
> >>>
> >>> This feature has to be explicit enabled by buses/devices supporting it.
> >>
> >> buses?
I think I should have called it 'interface'.
> >>
> >>>
> >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >>> ---
> >>> block.c | 18 ++++++++++++++++++
> >>> block.h | 7 +++++++
> >>> block_int.h | 2 ++
> >>> 3 files changed, 27 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index 24a25d5..cc0a34e 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -195,6 +195,7 @@ BlockDriverState *bdrv_new(const char *device_name)
> >>> if (device_name[0] != '\0') {
> >>> QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> >>> }
> >>> + bs->iostatus_enabled = false;
> >>> return bs;
> >>> }
> >>>
> >>> @@ -2876,6 +2877,23 @@ int bdrv_in_use(BlockDriverState *bs)
> >>> return bs->in_use;
> >>> }
> >>>
> >>> +void bdrv_enable_iostatus(BlockDriverState *bs)
> >>> +{
> >>> + bs->iostatus_enabled = true;
> >>> +}
> >>> +
> >>> +void bdrv_iostatus_update(BlockDriverState *bs, int error)
> >>> +{
> >>> + error = abs(error);
> >>> +
> >>> + if (!error) {
> >>> + bs->iostatus = BDRV_IOS_OK;
> >>> + } else {
> >>> + bs->iostatus = (error == ENOSPC) ? BDRV_IOS_ENOSPC :
> >>> + BDRV_IOS_FAILED;
> >>> + }
> >>> +}
> >>> +
> >>> int bdrv_img_create(const char *filename, const char *fmt,
> >>> const char *base_filename, const char *base_fmt,
> >>> char *options, uint64_t img_size, int flags)
> >>> diff --git a/block.h b/block.h
> >>> index 859d1d9..0dca1bb 100644
> >>> --- a/block.h
> >>> +++ b/block.h
> >>> @@ -50,6 +50,13 @@ typedef enum {
> >>> BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> >>> } BlockMonEventAction;
> >>>
> >>> +typedef enum {
> >>> + BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
> >>> +} BlockIOStatus;
> >>> +
> >>> +void bdrv_iostatus_update(BlockDriverState *bs, int error);
> >>> +void bdrv_enable_iostatus(BlockDriverState *bs);
> >>> +void bdrv_enable_io_status(BlockDriverState *bs);
> >>> void bdrv_mon_event(const BlockDriverState *bdrv,
> >>> BlockMonEventAction action, int is_read);
> >>> void bdrv_info_print(Monitor *mon, const QObject *data);
> >>> diff --git a/block_int.h b/block_int.h
> >>> index 1e265d2..09f038d 100644
> >>> --- a/block_int.h
> >>> +++ b/block_int.h
> >>> @@ -195,6 +195,8 @@ struct BlockDriverState {
> >>> drivers. They are not used by the block driver */
> >>> int cyls, heads, secs, translation;
> >>> BlockErrorAction on_read_error, on_write_error;
> >>> + bool iostatus_enabled;
> >>> + BlockIOStatus iostatus;
> >>> char device_name[32];
> >>> unsigned long *dirty_bitmap;
> >>> int64_t dirty_count;
> >>
> >> Okay, let's see what we got here.
> >>
> >> The block layer merely holds I/O status, device models set it.
> >>
> >> Device I/O status is not migrated. Why?
Bug. :)
> >>
> >> bdrv_new() creates the BDS with I/O status tracking disabled. Devices
> >> that do tracking enable it in their qdev init method. If a device gets
> >> hot unplugged, tracking remains enabled. If the BDS then gets reused
> >> with a device that doesn't do tracking, I/O status becomes incorrect.
> >> Can't happen right now, because we automatically delete the BDS on hot
> >> unplug, but it's a trap. Suggest to disable tracking in bdrv_detach().
> >>
> >> Actually, this is a symptom of the midlayer disease. I suspect things
> >> would be simpler if we hold the status in its rightful owner, the device
> >> model. Need a getter for it. I'm working on a patch series that moves
> >> misplaced state out of the block layer into device models and block
> >> drivers, and a I/O status getter will fit in easily there.
Excellent.
> >
> > This is host state, so the device is not the rightful owner. Devices
> > should not even be involved with enabling it.
>
> They are because they do the tracking, and thus the tracking only works
> for device models that do it. Could it be done entirely within the
> block layer?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-05 18:17 ` [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status Luiz Capitulino
2011-07-12 7:45 ` Markus Armbruster
@ 2011-07-12 14:25 ` Kevin Wolf
2011-07-12 14:56 ` Luiz Capitulino
1 sibling, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2011-07-12 14:25 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: stefanha, jan.kiszka, jdenemar, qemu-devel, armbru
Am 05.07.2011 20:17, schrieb Luiz Capitulino:
> This commit adds support to the BlockDriverState type to keep track
> of the last I/O status. That is, at every I/O operation we update
> a status field in the BlockDriverState instance. Valid statuses are:
> OK, FAILED and ENOSPC.
>
> ENOSPC is distinguished from FAILED because an management application
> can use it to implement thin-provisioning.
>
> This feature has to be explicit enabled by buses/devices supporting it.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
I'm not sure how this is meant to work with devices that can have
multiple requests in flight. If a request fails, one of the things that
are done before sending a monitor event is qemu_aio_flush(), i.e.
waiting for all in-flight requests to complete. If the last one of them
is successful, your status will report BDRV_IOS_OK.
If you don't stop the VM on I/O errors, the status is useless anyway,
even if only one request is active at the same point.
I think it would make more sense if we only stored the last error (that
is, don't clear the field on success). What is the use case, would this
be enough for it?
By the way, I'm not sure how it fits in, but I'd like to have a block
layer function that format drivers can use to tell qemu that the image
is corrupted. Maybe that's another case in which we should stop the VM
and have an appropriate status for it. It should probably have
precedence over an ENOSPC happening at the same time, so maybe we'll
also need a way to tell that some status is more important and may
overwrite a less important status, but not the other way round.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status
2011-07-12 14:25 ` Kevin Wolf
@ 2011-07-12 14:56 ` Luiz Capitulino
0 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 14:56 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, jan.kiszka, jdenemar, qemu-devel, armbru
On Tue, 12 Jul 2011 16:25:22 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 05.07.2011 20:17, schrieb Luiz Capitulino:
> > This commit adds support to the BlockDriverState type to keep track
> > of the last I/O status. That is, at every I/O operation we update
> > a status field in the BlockDriverState instance. Valid statuses are:
> > OK, FAILED and ENOSPC.
> >
> > ENOSPC is distinguished from FAILED because an management application
> > can use it to implement thin-provisioning.
> >
> > This feature has to be explicit enabled by buses/devices supporting it.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>
> I'm not sure how this is meant to work with devices that can have
> multiple requests in flight. If a request fails, one of the things that
> are done before sending a monitor event is qemu_aio_flush(), i.e.
> waiting for all in-flight requests to complete. If the last one of them
> is successful, your status will report BDRV_IOS_OK.
We're more interested in states that the device can not recover from or
that are not temporary. So, if something really bad happens I'd expect
all in-flight requests to fail the same way. Am I wrong?
> If you don't stop the VM on I/O errors, the status is useless anyway,
> even if only one request is active at the same point.
Right, that's a good point. A mngt application can only trust that the
status won't change in the next second if the VM is stopped.
> I think it would make more sense if we only stored the last error (that
> is, don't clear the field on success). What is the use case, would this
> be enough for it?
Yes, it would, but there's a problem. If the management application manages
to correct the error and put the VM to run again, we need to clear the status,
otherwise the management application could get confused if the status is read
at a later time.
The most effective way I found to do this was to let the device report its
own current status. But I see two other ways of doing this:
1. We could only report the status if the VM is paused. This doesn't change
much the implementation though
2. We could allow the mngt app to clear the status
> By the way, I'm not sure how it fits in, but I'd like to have a block
> layer function that format drivers can use to tell qemu that the image
> is corrupted. Maybe that's another case in which we should stop the VM
> and have an appropriate status for it. It should probably have
> precedence over an ENOSPC happening at the same time, so maybe we'll
> also need a way to tell that some status is more important and may
> overwrite a less important status, but not the other way round.
Yes, seems to make sense.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 4/8] ide: Support I/O status
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
` (2 preceding siblings ...)
2011-07-05 18:17 ` [Qemu-devel] [PATCH 3/8] block: Support to keep track of I/O status Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 5/8] virtio: " Luiz Capitulino
` (4 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
hw/ide/core.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index bf9df41..5fae4c6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -473,6 +473,7 @@ void ide_sector_read(IDEState *s)
if (n > s->req_nb_sectors)
n = s->req_nb_sectors;
ret = bdrv_read(s->bs, sector_num, s->io_buffer, n);
+ bdrv_iostatus_update(s->bs, ret);
if (ret != 0) {
if (ide_handle_rw_error(s, -ret,
BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ))
@@ -544,6 +545,7 @@ void ide_dma_cb(void *opaque, int ret)
int64_t sector_num;
handle_rw_error:
+ bdrv_iostatus_update(s->bs, ret);
if (ret < 0) {
int op = BM_STATUS_DMA_RETRY;
@@ -642,6 +644,7 @@ void ide_sector_write(IDEState *s)
if (n > s->req_nb_sectors)
n = s->req_nb_sectors;
ret = bdrv_write(s->bs, sector_num, s->io_buffer, n);
+ bdrv_iostatus_update(s->bs, ret);
if (ret != 0) {
if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY))
@@ -678,6 +681,7 @@ static void ide_flush_cb(void *opaque, int ret)
{
IDEState *s = opaque;
+ bdrv_iostatus_update(s->bs, ret);
if (ret < 0) {
/* XXX: What sector number to set here? */
if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
@@ -1749,6 +1753,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
}
ide_reset(s);
+ bdrv_enable_iostatus(bs);
bdrv_set_removable(bs, s->drive_kind == IDE_CD);
return 0;
}
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 5/8] virtio: Support I/O status
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
` (3 preceding siblings ...)
2011-07-05 18:17 ` [Qemu-devel] [PATCH 4/8] ide: Support " Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 6/8] scsi: " Luiz Capitulino
` (3 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
hw/virtio-blk.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index bf70200..692520c 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -94,6 +94,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
trace_virtio_blk_rw_complete(req, ret);
+ bdrv_iostatus_update(req->dev->bs, ret);
if (ret) {
int is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
if (virtio_blk_handle_rw_error(req, -ret, is_read))
@@ -107,6 +108,7 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
{
VirtIOBlockReq *req = opaque;
+ bdrv_iostatus_update(req->dev->bs, ret);
if (ret) {
if (virtio_blk_handle_rw_error(req, -ret, 0)) {
return;
@@ -577,6 +579,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
bdrv_set_change_cb(s->bs, virtio_blk_change_cb, s);
s->bs->buffer_alignment = conf->logical_block_size;
+ bdrv_enable_iostatus(s->bs);
add_boot_device_path(conf->bootindex, dev, "/disk@0,0");
return &s->vdev;
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 6/8] scsi: Support I/O status
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
` (4 preceding siblings ...)
2011-07-05 18:17 ` [Qemu-devel] [PATCH 5/8] virtio: " Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-05 18:17 ` [Qemu-devel] [PATCH 7/8] QMP: query-status: Add 'io-status' key Luiz Capitulino
` (2 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
hw/scsi-disk.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 66037fd..7002560 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -137,10 +137,12 @@ static void scsi_cancel_io(SCSIRequest *req)
static void scsi_read_complete(void * opaque, int ret)
{
SCSIDiskReq *r = (SCSIDiskReq *)opaque;
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
int n;
r->req.aiocb = NULL;
+ bdrv_iostatus_update(s->bs, ret);
if (ret) {
if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_READ)) {
return;
@@ -243,11 +245,13 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
static void scsi_write_complete(void * opaque, int ret)
{
SCSIDiskReq *r = (SCSIDiskReq *)opaque;
+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
uint32_t len;
uint32_t n;
r->req.aiocb = NULL;
+ bdrv_iostatus_update(s->bs, ret);
if (ret) {
if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_WRITE)) {
return;
@@ -905,6 +909,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
break;
case SYNCHRONIZE_CACHE:
ret = bdrv_flush(s->bs);
+ bdrv_iostatus_update(s->bs, ret);
if (ret < 0) {
if (scsi_handle_rw_error(r, -ret, SCSI_REQ_STATUS_RETRY_FLUSH)) {
return -1;
@@ -1226,6 +1231,7 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
s->qdev.type = TYPE_DISK;
qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
+ bdrv_enable_iostatus(s->bs);
bdrv_set_removable(s->bs, kind == SCSI_CD);
add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
return 0;
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH 7/8] QMP: query-status: Add 'io-status' key
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
` (5 preceding siblings ...)
2011-07-05 18:17 ` [Qemu-devel] [PATCH 6/8] scsi: " Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-12 7:47 ` Markus Armbruster
2011-07-05 18:17 ` [Qemu-devel] [PATCH 8/8] HMP: Print 'io-status' information Luiz Capitulino
2011-07-11 17:43 ` [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
8 siblings, 1 reply; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
Contains the last I/O status for the given device. Currently this is
only supported by ide, scsi and virtio block devices.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
block.c | 15 ++++++++++++++-
block.h | 2 +-
qmp-commands.hx | 6 ++++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index cc0a34e..28df3d8 100644
--- a/block.c
+++ b/block.c
@@ -1720,6 +1720,12 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
}
+static const char *const io_status_name[BDRV_IOS_MAX] = {
+ [BDRV_IOS_OK] = "ok",
+ [BDRV_IOS_FAILED] = "failed",
+ [BDRV_IOS_ENOSPC] = "nospace",
+};
+
void bdrv_info(Monitor *mon, QObject **ret_data)
{
QList *bs_list;
@@ -1729,15 +1735,16 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
QTAILQ_FOREACH(bs, &bdrv_states, list) {
QObject *bs_obj;
+ QDict *bs_dict;
bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
"'removable': %i, 'locked': %i }",
bs->device_name, bs->removable,
bs->locked);
+ bs_dict = qobject_to_qdict(bs_obj);
if (bs->drv) {
QObject *obj;
- QDict *bs_dict = qobject_to_qdict(bs_obj);
obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
"'encrypted': %i }",
@@ -1752,6 +1759,12 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
qdict_put_obj(bs_dict, "inserted", obj);
}
+
+ if (bs->iostatus_enabled) {
+ qdict_put(bs_dict, "io-status",
+ qstring_from_str(io_status_name[bs->iostatus]));
+ }
+
qlist_append_obj(bs_list, bs_obj);
}
diff --git a/block.h b/block.h
index 0dca1bb..0141fe6 100644
--- a/block.h
+++ b/block.h
@@ -51,7 +51,7 @@ typedef enum {
} BlockMonEventAction;
typedef enum {
- BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
+ BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC, BDRV_IOS_MAX
} BlockIOStatus;
void bdrv_iostatus_update(BlockDriverState *bs, int error);
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6b8eb0a..1746b6d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1082,6 +1082,9 @@ Each json-object contain the following:
"tftp", "vdi", "vmdk", "vpc", "vvfat"
- "backing_file": backing file name (json-string, optional)
- "encrypted": true if encrypted, false otherwise (json-bool)
+- "io-status": last executed I/O operation status, only present if the device
+ supports it (json_string, optional)
+ - Possible values: "ok", "failed", "nospace"
Example:
@@ -1089,6 +1092,7 @@ Example:
<- {
"return":[
{
+ "io-status": "ok",
"device":"ide0-hd0",
"locked":false,
"removable":false,
@@ -1101,12 +1105,14 @@ Example:
"type":"unknown"
},
{
+ "io-status": "ok",
"device":"ide1-cd0",
"locked":false,
"removable":true,
"type":"unknown"
},
{
+ "io-status": "ok",
"device":"floppy0",
"locked":false,
"removable":true,
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 7/8] QMP: query-status: Add 'io-status' key
2011-07-05 18:17 ` [Qemu-devel] [PATCH 7/8] QMP: query-status: Add 'io-status' key Luiz Capitulino
@ 2011-07-12 7:47 ` Markus Armbruster
2011-07-12 14:56 ` Luiz Capitulino
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2011-07-12 7:47 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, jan.kiszka, jdenemar, qemu-devel, stefanha
Luiz Capitulino <lcapitulino@redhat.com> writes:
> Contains the last I/O status for the given device. Currently this is
> only supported by ide, scsi and virtio block devices.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> block.c | 15 ++++++++++++++-
> block.h | 2 +-
> qmp-commands.hx | 6 ++++++
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index cc0a34e..28df3d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -1720,6 +1720,12 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
> qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
> }
>
> +static const char *const io_status_name[BDRV_IOS_MAX] = {
> + [BDRV_IOS_OK] = "ok",
> + [BDRV_IOS_FAILED] = "failed",
> + [BDRV_IOS_ENOSPC] = "nospace",
> +};
> +
> void bdrv_info(Monitor *mon, QObject **ret_data)
> {
> QList *bs_list;
> @@ -1729,15 +1735,16 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>
> QTAILQ_FOREACH(bs, &bdrv_states, list) {
> QObject *bs_obj;
> + QDict *bs_dict;
>
> bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> "'removable': %i, 'locked': %i }",
> bs->device_name, bs->removable,
> bs->locked);
> + bs_dict = qobject_to_qdict(bs_obj);
>
> if (bs->drv) {
> QObject *obj;
> - QDict *bs_dict = qobject_to_qdict(bs_obj);
>
> obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
> "'encrypted': %i }",
> @@ -1752,6 +1759,12 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>
> qdict_put_obj(bs_dict, "inserted", obj);
> }
> +
> + if (bs->iostatus_enabled) {
> + qdict_put(bs_dict, "io-status",
> + qstring_from_str(io_status_name[bs->iostatus]));
> + }
> +
> qlist_append_obj(bs_list, bs_obj);
> }
>
> diff --git a/block.h b/block.h
> index 0dca1bb..0141fe6 100644
> --- a/block.h
> +++ b/block.h
> @@ -51,7 +51,7 @@ typedef enum {
> } BlockMonEventAction;
>
> typedef enum {
> - BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
> + BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC, BDRV_IOS_MAX
> } BlockIOStatus;
>
> void bdrv_iostatus_update(BlockDriverState *bs, int error);
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6b8eb0a..1746b6d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1082,6 +1082,9 @@ Each json-object contain the following:
> "tftp", "vdi", "vmdk", "vpc", "vvfat"
> - "backing_file": backing file name (json-string, optional)
> - "encrypted": true if encrypted, false otherwise (json-bool)
> +- "io-status": last executed I/O operation status, only present if the device
> + supports it (json_string, optional)
> + - Possible values: "ok", "failed", "nospace"
>
> Example:
>
> @@ -1089,6 +1092,7 @@ Example:
> <- {
> "return":[
> {
> + "io-status": "ok",
> "device":"ide0-hd0",
> "locked":false,
> "removable":false,
> @@ -1101,12 +1105,14 @@ Example:
> "type":"unknown"
> },
> {
> + "io-status": "ok",
> "device":"ide1-cd0",
> "locked":false,
> "removable":true,
> "type":"unknown"
> },
> {
> + "io-status": "ok",
> "device":"floppy0",
> "locked":false,
> "removable":true,
floppy doesn't support I/O status, yet the example shows "io-status":
"ok". Are you sure it's correct?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH 7/8] QMP: query-status: Add 'io-status' key
2011-07-12 7:47 ` Markus Armbruster
@ 2011-07-12 14:56 ` Luiz Capitulino
0 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-12 14:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, jan.kiszka, jdenemar, qemu-devel, stefanha
On Tue, 12 Jul 2011 09:47:19 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
> > Contains the last I/O status for the given device. Currently this is
> > only supported by ide, scsi and virtio block devices.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> > block.c | 15 ++++++++++++++-
> > block.h | 2 +-
> > qmp-commands.hx | 6 ++++++
> > 3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index cc0a34e..28df3d8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1720,6 +1720,12 @@ void bdrv_info_print(Monitor *mon, const QObject *data)
> > qlist_iter(qobject_to_qlist(data), bdrv_print_dict, mon);
> > }
> >
> > +static const char *const io_status_name[BDRV_IOS_MAX] = {
> > + [BDRV_IOS_OK] = "ok",
> > + [BDRV_IOS_FAILED] = "failed",
> > + [BDRV_IOS_ENOSPC] = "nospace",
> > +};
> > +
> > void bdrv_info(Monitor *mon, QObject **ret_data)
> > {
> > QList *bs_list;
> > @@ -1729,15 +1735,16 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
> >
> > QTAILQ_FOREACH(bs, &bdrv_states, list) {
> > QObject *bs_obj;
> > + QDict *bs_dict;
> >
> > bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> > "'removable': %i, 'locked': %i }",
> > bs->device_name, bs->removable,
> > bs->locked);
> > + bs_dict = qobject_to_qdict(bs_obj);
> >
> > if (bs->drv) {
> > QObject *obj;
> > - QDict *bs_dict = qobject_to_qdict(bs_obj);
> >
> > obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
> > "'encrypted': %i }",
> > @@ -1752,6 +1759,12 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
> >
> > qdict_put_obj(bs_dict, "inserted", obj);
> > }
> > +
> > + if (bs->iostatus_enabled) {
> > + qdict_put(bs_dict, "io-status",
> > + qstring_from_str(io_status_name[bs->iostatus]));
> > + }
> > +
> > qlist_append_obj(bs_list, bs_obj);
> > }
> >
> > diff --git a/block.h b/block.h
> > index 0dca1bb..0141fe6 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -51,7 +51,7 @@ typedef enum {
> > } BlockMonEventAction;
> >
> > typedef enum {
> > - BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC
> > + BDRV_IOS_OK, BDRV_IOS_FAILED, BDRV_IOS_ENOSPC, BDRV_IOS_MAX
> > } BlockIOStatus;
> >
> > void bdrv_iostatus_update(BlockDriverState *bs, int error);
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 6b8eb0a..1746b6d 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -1082,6 +1082,9 @@ Each json-object contain the following:
> > "tftp", "vdi", "vmdk", "vpc", "vvfat"
> > - "backing_file": backing file name (json-string, optional)
> > - "encrypted": true if encrypted, false otherwise (json-bool)
> > +- "io-status": last executed I/O operation status, only present if the device
> > + supports it (json_string, optional)
> > + - Possible values: "ok", "failed", "nospace"
> >
> > Example:
> >
> > @@ -1089,6 +1092,7 @@ Example:
> > <- {
> > "return":[
> > {
> > + "io-status": "ok",
> > "device":"ide0-hd0",
> > "locked":false,
> > "removable":false,
> > @@ -1101,12 +1105,14 @@ Example:
> > "type":"unknown"
> > },
> > {
> > + "io-status": "ok",
> > "device":"ide1-cd0",
> > "locked":false,
> > "removable":true,
> > "type":"unknown"
> > },
> > {
> > + "io-status": "ok",
> > "device":"floppy0",
> > "locked":false,
> > "removable":true,
>
> floppy doesn't support I/O status, yet the example shows "io-status":
> "ok". Are you sure it's correct?
Good catch, I did this by hand :-)
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH 8/8] HMP: Print 'io-status' information
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
` (6 preceding siblings ...)
2011-07-05 18:17 ` [Qemu-devel] [PATCH 7/8] QMP: query-status: Add 'io-status' key Luiz Capitulino
@ 2011-07-05 18:17 ` Luiz Capitulino
2011-07-11 17:43 ` [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
8 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-05 18:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jan.kiszka, armbru, stefanha, jdenemar
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
block.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/block.c b/block.c
index 28df3d8..2b414ec 100644
--- a/block.c
+++ b/block.c
@@ -1695,6 +1695,10 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
}
+ if (qdict_haskey(bs_dict, "io-status")) {
+ monitor_printf(mon, " io-status=%s", qdict_get_str(bs_dict, "io-status"));
+ }
+
if (qdict_haskey(bs_dict, "inserted")) {
QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
--
1.7.6.131.g99019
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support
2011-07-05 18:17 [Qemu-devel] [PATCH v1 0/8]: QMP: Thin provisioning support Luiz Capitulino
` (7 preceding siblings ...)
2011-07-05 18:17 ` [Qemu-devel] [PATCH 8/8] HMP: Print 'io-status' information Luiz Capitulino
@ 2011-07-11 17:43 ` Luiz Capitulino
8 siblings, 0 replies; 29+ messages in thread
From: Luiz Capitulino @ 2011-07-11 17:43 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, jan.kiszka, qemu-devel, armbru, stefanha, jdenemar
On Tue, 5 Jul 2011 15:17:43 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Roughly speaking, thin provisioning is a feature where the VM is started with
> a small storage and when a no space error is triggered, more space is allocated
> and the VM is put to run again.
block guys ping?
This series has some block layer changes which would be nice to get feedback
from you.
>
> This series allows a management tool using QMP to implement thin provisioning
> support. It does the following:
>
> 1. patches 1/8 and 2/8 extend the query-status command to contain a more
> complete and descriptive status
>
> 2. patches 3/8 to 6/8 add support to the block layer to track the status of
> the last executed I/O operation. This is supported by ide, virtio and scsi
> devices
>
> 3. The last two patches extend the "query-block" and "info block" commands
> to print the last I/O status field (this is per device)
>
> Basically, all a management tool has to do to implement thin provisioning is
> to wait for a BLOCK_IO_ERROR event (or the STOP event), check the VM is
> stopped by issuing query-status and then find which device failed by using
> query-block. Of course that the VM has to be configured to stop on errors.
>
> A last important detail: Anthony has proposed how the query-status command
> should be extended to support this[1]. However, I had to make the following
> changes to his original proposal:
>
> - Added states: debug, inmigrate, load-state-error and internal-error
> - Dropped: singlestep
>
> You'll find more details in the patches, thanks!
>
> [1] http://lists.nongnu.org/archive/html/qemu-devel/2011-06/msg00352.html
>
> block.c | 37 ++++++++++++++++++++++++++++++++++++-
> block.h | 7 +++++++
> block_int.h | 2 ++
> gdbstub.c | 4 ++++
> hw/ide/core.c | 6 ++++++
> hw/scsi-disk.c | 7 +++++++
> hw/virtio-blk.c | 4 ++++
> hw/watchdog.c | 1 +
> kvm-all.c | 1 +
> migration.c | 3 +++
> monitor.c | 8 +++++---
> qmp-commands.hx | 23 ++++++++++++++++++++++-
> sysemu.h | 19 +++++++++++++++++++
> vl.c | 37 +++++++++++++++++++++++++++++++++++++
> 14 files changed, 154 insertions(+), 5 deletions(-)
>
^ permalink raw reply [flat|nested] 29+ messages in thread