[Qemu-devel] Re: [PATCH] loadvm: improve tests before bdrv_snapshot_goto()

[Kevin Wolf <kwolf@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 <miguel.filho@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