All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3] savevm: Really verify if a drive supports snapshots
@ 2010-06-03 19:52 Miguel Di Ciurcio Filho
  2010-06-04 13:57 ` [Qemu-devel] " Kevin Wolf
  2010-06-04 17:28 ` Kevin Wolf
  0 siblings, 2 replies; 4+ messages in thread
From: Miguel Di Ciurcio Filho @ 2010-06-03 19:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, armbru

Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.

First issue: Their names implies different porpouses, but they do the same thing
and have exactly the same code. Maybe copied and pasted and forgotten?
bdrv_has_snapshot() is called in various places for actually checking if there
is snapshots or not.

Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or
not snapshots does not catch all cases. E.g.: a raw image.

So when do_savevm() is called, first thing it does is to set a global
BlockDriverState to save the VM memory state calling get_bs_snapshots().

static BlockDriverState *get_bs_snapshots(void)
{
    BlockDriverState *bs;
    DriveInfo *dinfo;

    if (bs_snapshots)
        return bs_snapshots;
    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs = dinfo->bdrv;
        if (bdrv_can_snapshot(bs))
            goto ok;
    }
    return NULL;
 ok:
    bs_snapshots = bs;
    return bs;
}

bdrv_can_snapshot() may return a BlockDriverState that does not support
snapshots and do_savevm() goes on.

Later on in do_savevm(), we find:

    QTAILQ_FOREACH(dinfo, &drives, next) {
        bs1 = dinfo->bdrv;
        if (bdrv_has_snapshot(bs1)) {
            /* Write VM state size only to the image that contains the state */
            sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
            ret = bdrv_snapshot_create(bs1, sn);
            if (ret < 0) {
                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                               bdrv_get_device_name(bs1));
            }
        }
    }

bdrv_has_snapshot(bs1) is not checking if the device does support or has
snapshots as explained above. Only in bdrv_snapshot_create() the device is
actually checked for snapshot support.

So, in cases where the first device supports snapshots, and the second does not,
the snapshot on the first will happen anyways. I believe this is not a good
behavior. It should be an all or nothing process.

This patch addresses these issues by making bdrv_can_snapshot() actually do
what it must do and enforces better tests to avoid errors in the middle of
do_savevm(). bdrv_has_snapshot() is removed and replaced by bdrv_can_snapshot()
where appropriate.

bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense to me.

The loadvm_state() function was updated too to enforce that when loading a VM at
least all writable devices must support snapshots too.

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 block.c  |   11 +++++++++++
 block.h  |    1 +
 savevm.c |   58 ++++++++++++++++++++++++++++++++++++----------------------
 3 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index cd70730..ace3cdb 100644
--- a/block.c
+++ b/block.c
@@ -1720,6 +1720,17 @@ void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
 /**************************************************************/
 /* handling of snapshots */
 
+int bdrv_can_snapshot(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
+        bdrv_is_read_only(bs)) {
+        return 0;
+    }
+
+    return 1;
+}
+
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info)
 {
diff --git a/block.h b/block.h
index 24efeb6..fbcd8af 100644
--- a/block.h
+++ b/block.h
@@ -173,6 +173,7 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
+int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index dc20390..6549ca7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1574,22 +1574,6 @@ out:
     return ret;
 }
 
-/* device can contain snapshots */
-static int bdrv_can_snapshot(BlockDriverState *bs)
-{
-    return (bs &&
-            !bdrv_is_removable(bs) &&
-            !bdrv_is_read_only(bs));
-}
-
-/* device must be snapshots in order to have a reliable snapshot */
-static int bdrv_has_snapshot(BlockDriverState *bs)
-{
-    return (bs &&
-            !bdrv_is_removable(bs) &&
-            !bdrv_is_read_only(bs));
-}
-
 static BlockDriverState *get_bs_snapshots(void)
 {
     BlockDriverState *bs;
@@ -1599,8 +1583,9 @@ static BlockDriverState *get_bs_snapshots(void)
         return bs_snapshots;
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs = dinfo->bdrv;
-        if (bdrv_can_snapshot(bs))
+        if (bdrv_can_snapshot(bs)) {
             goto ok;
+        }
     }
     return NULL;
  ok:
@@ -1674,12 +1659,26 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 #endif
     const char *name = qdict_get_try_str(qdict, "name");
 
+    /* Verify if there is a device that doesn't support snapshots and is writable */
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        bs = dinfo->bdrv;
+
+        if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+            continue;
+        }
+
+        if (!bdrv_can_snapshot(bs)) {
+            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
+                               bdrv_get_device_name(bs));
+            goto the_end;
+        }
+    }
+
     bs = get_bs_snapshots();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
     }
-
     /* ??? Should this occur after vm_stop?  */
     qemu_aio_flush();
 
@@ -1732,7 +1731,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
             ret = bdrv_snapshot_create(bs1, sn);
@@ -1756,6 +1755,21 @@ int load_vmstate(const char *name)
     QEMUFile *f;
     int ret;
 
+    /* Verify if there is a device that doesn't support snapshots and is writable */
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        bs = dinfo->bdrv;
+
+        if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+            continue;
+        }
+
+        if (!bdrv_can_snapshot(bs)) {
+            error_report("Device '%s' is writable but does not support snapshots.",
+                               bdrv_get_device_name(bs));
+            return -ENOTSUP;
+        }
+    }
+
     bs = get_bs_snapshots();
     if (!bs) {
         error_report("No block device supports snapshots");
@@ -1767,7 +1781,7 @@ int load_vmstate(const char *name)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_can_snapshot(bs1)) {
             ret = bdrv_snapshot_goto(bs1, name);
             if (ret < 0) {
                 switch(ret) {
@@ -1829,7 +1843,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
 
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_can_snapshot(bs1)) {
             ret = bdrv_snapshot_delete(bs1, name);
             if (ret < 0) {
                 if (ret == -ENOTSUP)
@@ -1860,7 +1874,7 @@ void do_info_snapshots(Monitor *mon)
     monitor_printf(mon, "Snapshot devices:");
     QTAILQ_FOREACH(dinfo, &drives, next) {
         bs1 = dinfo->bdrv;
-        if (bdrv_has_snapshot(bs1)) {
+        if (bdrv_can_snapshot(bs1)) {
             if (bs == bs1)
                 monitor_printf(mon, " %s", bdrv_get_device_name(bs1));
         }
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-06-04 17:49 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-03 19:52 [Qemu-devel] [PATCH v3] savevm: Really verify if a drive supports snapshots Miguel Di Ciurcio Filho
2010-06-04 13:57 ` [Qemu-devel] " Kevin Wolf
2010-06-04 17:28 ` Kevin Wolf
2010-06-04 17:46   ` Miguel Di Ciurcio Filho

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.