On Tue, 12 Jul 2011 18:16:26 +0200
Kevin Wolf <kw...@redhat.com> wrote:
> Am 12.07.2011 18:03, schrieb Luiz Capitulino:
> > On Tue, 12 Jul 2011 12:12:31 -0300
> > Luiz Capitulino <lcapitul...@redhat.com> wrote:
> >
> >> On Tue, 12 Jul 2011 16:51:03 +0200
> >> Kevin Wolf <kw...@redhat.com> wrote:
> >>
> >>> 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.
> >>
> >> 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.
