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

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

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

Changelog from v1
---------------
- Use -ENOTSUP instead of -EINVAL when no device supports snapshots
- Split the verification of the existance of an snapshot on the VM state device
  and the verification of the size of the saved VM state

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 monitor.c |    3 +-
 savevm.c  |   71 +++++++++++++++++++++++++++++-------------------------------
 2 files changed, 36 insertions(+), 38 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..f21873e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1804,12 +1804,27 @@ 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 -ENOTSUP;
+    }
+
+    /* Don't even try to load empty VM states */
+    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+    if (ret < 0) {
+        return ret;
+    } else if (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))) {
 
@@ -1822,63 +1837,45 @@ int load_vmstate(const char *name)
                                bdrv_get_device_name(bs));
             return -ENOTSUP;
         }
-    }
 
-    bs = bdrv_snapshots();
-    if (!bs) {
-        error_report("No block device supports snapshots");
-        return -EINVAL;
+        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;
+        }
     }
 
     /* 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;
 }
 
-- 
1.7.1

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

[Kevin Wolf]

Am 19.07.2010 20:25, 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.
> 
> Changelog from v1
> ---------------
> - Use -ENOTSUP instead of -EINVAL when no device supports snapshots
> - Split the verification of the existance of an snapshot on the VM state device
>   and the verification of the size of the saved VM state
> 
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>

Thanks, applied to the block branch.

Kevin