From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:55686) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qgfaf-0005N3-0Y for qemu-devel@nongnu.org; Tue, 12 Jul 2011 12:13:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qgfac-0006cq-Vs for qemu-devel@nongnu.org; Tue, 12 Jul 2011 12:13:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qgfac-0006ch-Ce for qemu-devel@nongnu.org; Tue, 12 Jul 2011 12:13:38 -0400 Message-ID: <4E1C735A.6030705@redhat.com> Date: Tue, 12 Jul 2011 18:16:26 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1309889871-6267-1-git-send-email-lcapitulino@redhat.com> <1309889871-6267-2-git-send-email-lcapitulino@redhat.com> <20110712112552.5d71ae2c@doriath> <4E1C5F57.8020109@redhat.com> <20110712121231.1ce0072c@doriath> <20110712130322.061d6886@doriath> In-Reply-To: <20110712130322.061d6886@doriath> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: stefanha@gmail.com, Markus Armbruster , qemu-devel@nongnu.org, jan.kiszka@siemens.com, jdenemar@redhat.com Am 12.07.2011 18:03, schrieb Luiz Capitulino: > On Tue, 12 Jul 2011 12:12:31 -0300 > Luiz Capitulino wrote: > >> On Tue, 12 Jul 2011 16:51:03 +0200 >> Kevin Wolf wrote: >> >>> Am 12.07.2011 16:25, schrieb Luiz Capitulino: >>>> On Tue, 12 Jul 2011 09:28:05 +0200 >>>> Markus Armbruster wrote: >>>> >>>>> Luiz Capitulino 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 >>>>>> --- >>>>>> 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