[Qemu-devel] Combining "loadvm" and "-snapshot"

[Stephen McCamant]

Loading snapshots with "loadvm" and running with a scratch QCOW2 disk
image using "-snapshot" are both useful features, but they would be
even more valuable if they could be used together. For instance, I'd
like to have a disk image containing several snapshoted states, and to
be able to load any of them into a QEMU instance with "-snapshot" to
play with them without changing the base disk. But this doesn't seem
to be supported at the moment, since "loadvm" can only load from the
scratch image, not the one backing image.

I looked into an apparently unapplied patch that Eddie Kohler sent
last year that enables a related functionality: being able to load
snapshots from a disk image that's read-only.
(http://www.mail-archive.com/address@hidden/msg15774.html).
This targets a related application, but it seems less useful because
after loading the snapshot, the VM's disk is still read-only, so any
writes will fail with an IO error. This provides the most protection
for the original image, but limits what you can do. (It would still be
useful alongside what I propose below.)

I've tried implementing an alternate approach that makes "loadvm" work
under "-snapshot" by finding and loading the VM state from the backing
image, if a snapshot with that name isn't found in the scratch image.
In some preliminary testing, this seems to do what I want; you can
also make new snapshots with savevm that live only in the scratch
image (and so go away at the end of the session). I also made "info
snapshots" print snapshots from both images. Some remaining questions:

* Snapshot IDs aren't unique between the two images, which looks weird
  in the output of "info snapshots" and means you have to load by tag.

* You still can't copy snapshots back to the backing image using
  "commit".

The patch below shows what I've been working on. I've modified
bdrv_snapshot_goto with a new out parameter representing the
BlockDriverState to load the VM state from (potentially different from
the one passed as an argument).

 -- Stephen

diff --git a/block.c b/block.c
index 3250327..0fa7a28 100644
--- a/block.c
+++ b/block.c
@@ -1138,14 +1138,25 @@ int bdrv_snapshot_create(BlockDriverState *bs,
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id)
+                       const char *snapshot_id,
+                       BlockDriverState **vm_state_bs)
 {
     BlockDriver *drv = bs->drv;
+    int ret;
     if (!drv)
         return -ENOMEDIUM;
     if (!drv->bdrv_snapshot_goto)
         return -ENOTSUP;
-    return drv->bdrv_snapshot_goto(bs, snapshot_id);
+    ret = drv->bdrv_snapshot_goto(bs, snapshot_id);
+    if (ret == -ENOENT && bs->backing_hd && drv == bs->backing_hd->drv) {
+       /* Try loading a snapshot from the backing device */
+       if (vm_state_bs)
+           *vm_state_bs = bs->backing_hd;
+       return drv->bdrv_snapshot_goto(bs->backing_hd, snapshot_id);
+    }
+    if (vm_state_bs)
+       *vm_state_bs = bs;
+    return ret;
 }
 
 int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
@@ -1166,7 +1177,28 @@ int bdrv_snapshot_list(BlockDriverState *bs,
         return -ENOMEDIUM;
     if (!drv->bdrv_snapshot_list)
         return -ENOTSUP;
-    return drv->bdrv_snapshot_list(bs, psn_info);
+    if (bs->backing_hd) {
+       /* Combine snapshot lists from backing and local devices */
+       BlockDriverState *back_bs = bs->backing_hd;
+       BlockDriver *back_drv = back_bs->drv;
+       QEMUSnapshotInfo *backing_snaps, *local_snaps;
+       int backing_snap_cnt =
+           back_drv->bdrv_snapshot_list(back_bs, &backing_snaps);
+       int local_snap_cnt = drv->bdrv_snapshot_list(bs, &local_snaps);
+       int snap_cnt = backing_snap_cnt + local_snap_cnt;
+       QEMUSnapshotInfo *ret_snaps =
+           qemu_malloc(snap_cnt * sizeof(QEMUSnapshotInfo));
+       memcpy(ret_snaps, backing_snaps,
+              backing_snap_cnt * sizeof(QEMUSnapshotInfo));
+       memcpy(ret_snaps + backing_snap_cnt, local_snaps,
+              local_snap_cnt * sizeof(QEMUSnapshotInfo));
+       qemu_free(backing_snaps);
+       qemu_free(local_snaps);
+       *psn_info = ret_snaps;
+       return snap_cnt;
+    } else {
+       return drv->bdrv_snapshot_list(bs, psn_info);
+    }
 }
 
 #define NB_SUFFIXES 4
diff --git a/block.h b/block.h
index c3314a1..b064d21 100644
--- a/block.h
+++ b/block.h
@@ -146,7 +146,8 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id);
+                       const char *snapshot_id,
+                       BlockDriverState **vm_state_bs);
 int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
diff --git a/qemu-img.c b/qemu-img.c
index 555ab5f..161af2b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -832,7 +832,7 @@ static void img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_APPLY:
-        ret = bdrv_snapshot_goto(bs, snapshot_name);
+        ret = bdrv_snapshot_goto(bs, snapshot_name, NULL);
         if (ret)
             error("Could not apply snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 729e849..0b265ea 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1116,7 +1116,7 @@ void do_savevm(const char *name)
 
 void do_loadvm(const char *name)
 {
-    BlockDriverState *bs, *bs1;
+    BlockDriverState *bs, *bs1, *vm_state_bs;
     BlockDriverInfo bdi1, *bdi = &bdi1;
     QEMUSnapshotInfo sn;
     QEMUFile *f;
@@ -1138,7 +1138,7 @@ void do_loadvm(const char *name)
     for(i = 0; i <= nb_drives; i++) {
         bs1 = drives_table[i].bdrv;
         if (bdrv_has_snapshot(bs1)) {
-            ret = bdrv_snapshot_goto(bs1, name);
+            ret = bdrv_snapshot_goto(bs1, name, &vm_state_bs);
             if (ret < 0) {
                 if (bs != bs1)
                     term_printf("Warning: ");
@@ -1163,19 +1163,19 @@ void do_loadvm(const char *name)
         }
     }
 
-    if (bdrv_get_info(bs, bdi) < 0 || bdi->vm_state_offset <= 0) {
+    if (bdrv_get_info(vm_state_bs, bdi) < 0 || bdi->vm_state_offset <= 0) {
         term_printf("Device %s does not support VM state snapshots\n",
                     bdrv_get_device_name(bs));
         return;
     }
 
     /* Don't even try to load empty VM states */
-    ret = bdrv_snapshot_find(bs, &sn, name);
+    ret = bdrv_snapshot_find(vm_state_bs, &sn, name);
     if ((ret >= 0) && (sn.vm_state_size == 0))
         goto the_end;
 
     /* restore the VM state */
-    f = qemu_fopen_bdrv(bs, bdi->vm_state_offset, 0);
+    f = qemu_fopen_bdrv(vm_state_bs, bdi->vm_state_offset, 0);
     if (!f) {
         term_printf("Could not open VM state file\n");
         goto the_end;