Re: [Qemu-devel] [PATCH 1/8] Introduce the VMStatus type

[Luiz Capitulino <lcapitulino@redhat.com>]

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.