From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O4LUM-00067G-8j for qemu-devel@nongnu.org; Tue, 20 Apr 2010 18:00:14 -0400 Received: from [140.186.70.92] (port=36115 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O4LUK-00066y-Np for qemu-devel@nongnu.org; Tue, 20 Apr 2010 18:00:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O4LUI-0003Pq-QQ for qemu-devel@nongnu.org; Tue, 20 Apr 2010 18:00:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9808) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O4LUH-0003PR-DF for qemu-devel@nongnu.org; Tue, 20 Apr 2010 18:00:10 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o3KM07Y9027928 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 20 Apr 2010 18:00:08 -0400 Date: Tue, 20 Apr 2010 18:59:59 -0300 From: Luiz Capitulino Message-ID: <20100420185959.31829121@redhat.com> In-Reply-To: References: <1271797792-24571-1-git-send-email-lcapitulino@redhat.com> <1271797792-24571-5-git-send-email-lcapitulino@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 04/22] savevm: do_loadvm(): Always resume the VM List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: kwolf@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On Tue, 20 Apr 2010 23:28:23 +0200 Juan Quintela wrote: > Luiz Capitulino wrote: > > do_loadvm(), which implements the 'loadvm' Monitor command, pauses > > the emulation to load the saved VM, however it will only resume > > it if the loading succeeds. > > > > In other words, if the user issues 'loadvm' and it fails, the > > end result will be an error message and a paused VM. > > > > This seems an undesirable side effect to me because, most of the > > time, if a Monitor command fails the best thing we can do is to > > leave the VM as it were before the command was executed. > > > > FIXME: This will try to run a potentially corrupted image, the > > solution is to split load_vmstate() in two and only keep > > the VM paused if qemu_loadvm_state() fails. > > Any of the other errors in loadvm also requires you to not load the > state. Really? Everything that happens before qemu_fopen_bdrv() seems to be only looking for the snapshot.. > > > Signed-off-by: Luiz Capitulino > > Nack. > > This can cause disk corruption. You tried to load a vm image, failed > the load somehow (notice the somehow) and you try to run it anyways. > That is a recipe for disaster. If load_vmstate() fails -> you don't run. My understanding is that the loading only happens in qemu_loadvm_state(), is this wrong? If it isn't, my plan is to split load_vmstate() in two functions: - load_vmstate_prepare(): everything before qemu_fopen_bdrv() - load_vmstate_finish(): qemu_loadvm_state() block then, do_loadvm() would do: err = load_vmstate_prepare(); if (err && vm_running) { vm_start(); return -1; } err = load_vmstate_finish(); if (err) { return -1; } vm_start(); return 0; And load_vmstate() would just call prepare() and finish(), maintaining its current behavior. It's important to understand why this is needed: currently you have a 'return 0' in line savevm.c:1872. It's an error path, but I'm almost sure that this trick is needed because if you return -1 there and loadvm fails for stupid reasons (like a not found snapshot) you get a paused VM.