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 <miguel.fi...@gmail.com>
> ---
> 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
