Re: [Qemu-devel] [PATCH 2/3] cpus: Add return value for vm_stop()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 05/07/2013 14:03, Kevin Wolf ha scritto:
> If flushing the block devices fails, return an error. The VM is stopped
> anyway.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  cpus.c                  | 20 +++++++++++++-------
>  include/sysemu/sysemu.h |  4 ++--
>  stubs/vm-stop.c         |  2 +-
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 20958e5..e15fdcb 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -435,17 +435,21 @@ bool cpu_is_stopped(CPUState *cpu)
>      return !runstate_is_running() || cpu->stopped;
>  }
>  
> -static void do_vm_stop(RunState state)
> +static int do_vm_stop(RunState state)
>  {
> +    int ret = 0;
> +
>      if (runstate_is_running()) {
>          cpu_disable_ticks();
>          pause_all_vcpus();
>          runstate_set(state);
>          vm_state_notify(0, state);
>          bdrv_drain_all();
> -        bdrv_flush_all();
> +        ret = bdrv_flush_all();
>          monitor_protocol_event(QEVENT_STOP, NULL);
>      }
> +
> +    return ret;
>  }
>  
>  static bool cpu_can_run(CPUState *cpu)
> @@ -1078,7 +1082,7 @@ void cpu_stop_current(void)
>      }
>  }
>  
> -void vm_stop(RunState state)
> +int vm_stop(RunState state)
>  {
>      if (qemu_in_vcpu_thread()) {
>          qemu_system_vmstop_request(state);
> @@ -1087,19 +1091,21 @@ void vm_stop(RunState state)
>           * vm_stop() has been requested.
>           */
>          cpu_stop_current();
> -        return;
> +        return 0;
>      }
> -    do_vm_stop(state);
> +
> +    return do_vm_stop(state);
>  }
>  
>  /* does a state transition even if the VM is already stopped,
>     current state is forgotten forever */
> -void vm_stop_force_state(RunState state)
> +int vm_stop_force_state(RunState state)
>  {
>      if (runstate_is_running()) {
> -        vm_stop(state);
> +        return vm_stop(state);
>      } else {
>          runstate_set(state);

I think you should add a bdrv_flush_all() here.  Otherwise, you could
migrate a stopped VM that has failed to flush (not that unlikely if the
VM was stopped due to ENOSPC, for example).

Overall these patches fix the problem and they are good, but I even
wonder if the failure to flush should change the runstate to "I/O error"
and trigger a monitor event.

In the end, the "runstate" design is showing some problems...  I found
this discussion: http://patchwork.ozlabs.org/patch/117606/ and if you
search for "Paolo Bonzini - Oct. 6, 2011, 11:14 a.m." you can find my
humble proposal.

Paolo

> +        return 0;
>      }
>  }
>  
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 2fb71af..b5e1add 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -35,8 +35,8 @@ void vm_state_notify(int running, RunState state);
>  #define VMRESET_REPORT   true
>  
>  void vm_start(void);
> -void vm_stop(RunState state);
> -void vm_stop_force_state(RunState state);
> +int vm_stop(RunState state);
> +int vm_stop_force_state(RunState state);
>  
>  typedef enum WakeupReason {
>      QEMU_WAKEUP_REASON_OTHER = 0,
> diff --git a/stubs/vm-stop.c b/stubs/vm-stop.c
> index 4568935..f82c897 100644
> --- a/stubs/vm-stop.c
> +++ b/stubs/vm-stop.c
> @@ -1,7 +1,7 @@
>  #include "qemu-common.h"
>  #include "sysemu/sysemu.h"
>  
> -void vm_stop(RunState state)
> +int vm_stop(RunState state)
>  {
>      abort();
>  }
>