From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uv5JJ-0007qp-Kr for qemu-devel@nongnu.org; Fri, 05 Jul 2013 08:40:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uv5JI-00054B-C6 for qemu-devel@nongnu.org; Fri, 05 Jul 2013 08:40:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20372) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uv5JI-000547-5E for qemu-devel@nongnu.org; Fri, 05 Jul 2013 08:40:24 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r65CeMvC016230 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 5 Jul 2013 08:40:23 -0400 Message-ID: <51D6BEAE.4050502@redhat.com> Date: Fri, 05 Jul 2013 14:40:14 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1373025833-22859-1-git-send-email-kwolf@redhat.com> <1373025833-22859-3-git-send-email-kwolf@redhat.com> In-Reply-To: <1373025833-22859-3-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] cpus: Add return value for vm_stop() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, stefanha@redhat.com, quintela@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 > --- > 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(); > } >