Am 12.07.2011 16:25, schrieb Luiz Capitulino:
> On Tue, 12 Jul 2011 09:28:05 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitul...@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 <lcapitul...@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
