Patchwork loadvm: improve tests before bdrv_snapshot_goto()

login
register
mail settings
Submitter Miguel Di Ciurcio Filho
Date July 14, 2010, 6:27 p.m.
Message ID <1279132051-15865-1-git-send-email-miguel.filho@gmail.com>
Download mbox | patch
Permalink /patch/58921/
State New
Headers show

Comments

Miguel Di Ciurcio Filho - July 14, 2010, 6:27 p.m.
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(-)
Kevin Wolf - July 19, 2010, 2:22 p.m.
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
Miguel Di Ciurcio Filho - July 19, 2010, 6:02 p.m.
On Mon, Jul 19, 2010 at 11:22 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> -    /* 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?

It was -EINVAL before, just kept it. But -ENOTSUP make more sense.


>> +    /* 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;
> }
>

Better indeed.

>>
>> @@ -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.
>

Ack.

I will send v2 shortly.

Thanks,

Miguel

Patch

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;
+    }
+
+    /* 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;
+    }
+
+    /* 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 {
+            ret = bdrv_snapshot_find(bs, &sn, name);
+            if (ret < 0) {
+                error_report("Device '%s' does not have the requested snapshot '%s'",
+                               bdrv_get_device_name(bs), name);
+                return ret;
+            }
         }
     }
 
-    bs = bdrv_snapshots();
-    if (!bs) {
-        error_report("No block device supports snapshots");
-        return -EINVAL;
-    }
-
     /* Flush all IO requests so they don't interfere with the new state.  */
     qemu_aio_flush();
 
-    bs1 = NULL;
-    while ((bs1 = bdrv_next(bs1))) {
-        if (bdrv_can_snapshot(bs1)) {
-            ret = bdrv_snapshot_goto(bs1, name);
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
+        if (bdrv_can_snapshot(bs)) {
+            ret = bdrv_snapshot_goto(bs, name);
             if (ret < 0) {
-                switch(ret) {
-                case -ENOTSUP:
-                    error_report("%sSnapshots not supported on device '%s'",
-                                 bs != bs1 ? "Warning: " : "",
-                                 bdrv_get_device_name(bs1));
-                    break;
-                case -ENOENT:
-                    error_report("%sCould not find snapshot '%s' on device '%s'",
-                                 bs != bs1 ? "Warning: " : "",
-                                 name, bdrv_get_device_name(bs1));
-                    break;
-                default:
-                    error_report("%sError %d while activating snapshot on '%s'",
-                                 bs != bs1 ? "Warning: " : "",
-                                 ret, bdrv_get_device_name(bs1));
-                    break;
-                }
-                /* fatal on snapshot block device */
-                if (bs == bs1)
-                    return 0;
+                error_report("Error %d while activating snapshot '%s' on '%s'",
+                             ret, name, bdrv_get_device_name(bs));
+                return ret;
             }
         }
     }
 
-    /* Don't even try to load empty VM states */
-    ret = bdrv_snapshot_find(bs, &sn, name);
-    if ((ret >= 0) && (sn.vm_state_size == 0))
-        return -EINVAL;
-
     /* restore the VM state */
-    f = qemu_fopen_bdrv(bs, 0);
+    f = qemu_fopen_bdrv(bs_vm_state, 0);
     if (!f) {
         error_report("Could not open VM state file");
         return -EINVAL;
     }
+
     ret = qemu_loadvm_state(f);
+
     qemu_fclose(f);
     if (ret < 0) {
         error_report("Error %d while loading VM state", ret);
         return ret;
     }
+
     return 0;
 }