From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39842 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OarEk-0000LI-Gf for qemu-devel@nongnu.org; Mon, 19 Jul 2010 10:22:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OarEj-0005mL-5U for qemu-devel@nongnu.org; Mon, 19 Jul 2010 10:22:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32395) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OarEi-0005mB-Sk for qemu-devel@nongnu.org; Mon, 19 Jul 2010 10:22:29 -0400 Message-ID: <4C445F95.2090503@redhat.com> Date: Mon, 19 Jul 2010 16:22:13 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1279132051-15865-1-git-send-email-miguel.filho@gmail.com> In-Reply-To: <1279132051-15865-1-git-send-email-miguel.filho@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] loadvm: improve tests before bdrv_snapshot_goto() List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Miguel Di Ciurcio Filho Cc: aliguori@codemonkey.ws, qemu-devel@nongnu.org, lcapitulino@redhat.com Am 14.07.2010 20:27, schrieb Miguel Di Ciurcio Filho: > This patch improves the resilience of the load_vmstate() function, doing > further and better ordered tests. > > In load_vmstate(), if there is any error on bdrv_snapshot_goto(), except if the > error is on VM state device, load_vmstate() will return zero and the VM will be > started with major corruption chances. > > The current process: > - test if there is any writable device without snapshot support > - if exists return -error > - get the device that saves the VM state, possible return -error but unlikely > because it was tested earlier > - flush I/O > - run bdrv_snapshot_goto() on devices > - if fails, give an warning and goes to the next (not good!) > - if fails on the VM state device, return zero (not good!) > - check if the requested snapshot exists on the device that saves the VM state > and the state is not zero > - if fails return -error > - open the file with the VM state > - if fails return -error > - load the VM state > - if fails return -error > - return zero > > New behavior: > - get the device that saves the VM state > - if fails return -error > - check if the requested snapshot exists on the device that saves the VM state > and the state is not zero > - if fails return -error > - test if there is any writable device without snapshot support > - if exists return -error > - test if the devices with snapshot support have the requested snapshot > - if anyone fails, return -error > - flush I/O > - run snapshot_goto() on devices > - if anyone fails, return -error > - open the file with the VM state > - if fails return -error > - load the VM state > - if fails return -error > - return zero > > do_loadvm must not call vm_start if any error has occurred in load_vmstate. > > Signed-off-by: Miguel Di Ciurcio Filho > --- > monitor.c | 3 +- > savevm.c | 71 ++++++++++++++++++++++++++++-------------------------------- > 2 files changed, 35 insertions(+), 39 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 45fd482..aa60cfa 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2270,8 +2270,9 @@ static void do_loadvm(Monitor *mon, const QDict *qdict) > > vm_stop(0); > > - if (load_vmstate(name) >= 0 && saved_vm_running) > + if (load_vmstate(name) == 0 && saved_vm_running) { > vm_start(); > + } > } > > int monitor_get_fd(Monitor *mon, const char *fdname) > diff --git a/savevm.c b/savevm.c > index ee27989..9f29cb0 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1804,12 +1804,25 @@ void do_savevm(Monitor *mon, const QDict *qdict) > > int load_vmstate(const char *name) > { > - BlockDriverState *bs, *bs1; > + BlockDriverState *bs, *bs_vm_state; > QEMUSnapshotInfo sn; > QEMUFile *f; > int ret; > > - /* Verify if there is a device that doesn't support snapshots and is writable */ > + bs_vm_state = bdrv_snapshots(); > + if (!bs_vm_state) { > + error_report("No block device supports snapshots"); > + return -EINVAL; -ENOTSUP? > + } > + > + /* Don't even try to load empty VM states */ > + ret = bdrv_snapshot_find(bs_vm_state, &sn, name); > + if ((ret >= 0) && (sn.vm_state_size == 0)) { > + return -EINVAL; > + } You can probably stop here already if ret < 0: ret = ... if (ret < 0) { return ret; } else if (sn.vm_state_size == 0) { return -EINVAL; } I'm not sure about EINVAL here either, but I don't really have a better suggestion. > + > + /* Verify if there is any device that doesn't support snapshots and is > + writable and check if the requested snapshot is available too. */ > bs = NULL; > while ((bs = bdrv_next(bs))) { > > @@ -1821,64 +1834,46 @@ int load_vmstate(const char *name) > error_report("Device '%s' is writable but does not support snapshots.", > bdrv_get_device_name(bs)); > return -ENOTSUP; > + } else { The then branch has a return, so you don't need the else here and can have the following code nested one level less. Looks good otherwise. Kevin