[Qemu-devel] [PATCH] block: drop bs_snapshots global variable

[Qemu-devel] [PATCH] block: drop bs_snapshots global variable

[Stefan Hajnoczi]

The bs_snapshots global variable points to the BlockDriverState which
will be used to save vmstate.  This is really a savevm.c concept but was
moved into block.c:bdrv_snapshots() when it became clear that hotplug
could result in a dangling pointer.

While auditing the block layer's global state I came upon bs_snapshots
and realized that a variable is not necessary here.  Simply find the
first BlockDriverState capable of internal snapshots each time this is
needed.

The behavior of bdrv_snapshots() is preserved across hotplug because new
drives are always appended to the bdrv_states list.  This means that
calling the new find_vmstate_bs() function is idempotent - it returns
the same BlockDriverState unless it was hot-unplugged.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c               | 28 ----------------------------
 include/block/block.h |  1 -
 savevm.c              | 19 +++++++++++++++----
 3 files changed, 15 insertions(+), 33 deletions(-)

diff --git a/block.c b/block.c
index 124a9eb..0c6c612 100644
--- a/block.c
+++ b/block.c
@@ -99,9 +99,6 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
     QLIST_HEAD_INITIALIZER(bdrv_drivers);
 
-/* The device to use for VM snapshots */
-static BlockDriverState *bs_snapshots;
-
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -1169,9 +1166,6 @@ void bdrv_close(BlockDriverState *bs)
     notifier_list_notify(&bs->close_notifiers, bs);
 
     if (bs->drv) {
-        if (bs == bs_snapshots) {
-            bs_snapshots = NULL;
-        }
         if (bs->backing_hd) {
             bdrv_delete(bs->backing_hd);
             bs->backing_hd = NULL;
@@ -1402,7 +1396,6 @@ void bdrv_delete(BlockDriverState *bs)
 
     bdrv_close(bs);
 
-    assert(bs != bs_snapshots);
     g_free(bs);
 }
 
@@ -1446,9 +1439,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
 {
     bs->dev_ops = ops;
     bs->dev_opaque = opaque;
-    if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) {
-        bs_snapshots = NULL;
-    }
 }
 
 void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
@@ -3145,24 +3135,6 @@ int bdrv_is_snapshot(BlockDriverState *bs)
     return !!(bs->open_flags & BDRV_O_SNAPSHOT);
 }
 
-BlockDriverState *bdrv_snapshots(void)
-{
-    BlockDriverState *bs;
-
-    if (bs_snapshots) {
-        return bs_snapshots;
-    }
-
-    bs = NULL;
-    while ((bs = bdrv_next(bs))) {
-        if (bdrv_can_snapshot(bs)) {
-            bs_snapshots = bs;
-            return bs;
-        }
-    }
-    return NULL;
-}
-
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 0f750d7..c1b5650 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -329,7 +329,6 @@ BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
-BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index 147e2d2..1c0ec83 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2092,6 +2092,17 @@ out:
     return ret;
 }
 
+static BlockDriverState *find_vmstate_bs(void)
+{
+    BlockDriverState *bs = NULL;
+    while ((bs = bdrv_next(bs))) {
+        if (bdrv_can_snapshot(bs)) {
+            return bs;
+        }
+    }
+    return NULL;
+}
+
 static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                               const char *name)
 {
@@ -2168,7 +2179,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         }
     }
 
-    bs = bdrv_snapshots();
+    bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No block device can accept snapshots\n");
         return;
@@ -2270,7 +2281,7 @@ int load_vmstate(const char *name)
     QEMUFile *f;
     int ret;
 
-    bs_vm_state = bdrv_snapshots();
+    bs_vm_state = find_vmstate_bs();
     if (!bs_vm_state) {
         error_report("No block device supports snapshots");
         return -ENOTSUP;
@@ -2349,7 +2360,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     int ret;
     const char *name = qdict_get_str(qdict, "name");
 
-    bs = bdrv_snapshots();
+    bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No block device supports snapshots\n");
         return;
@@ -2381,7 +2392,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
     int *available_snapshots;
     char buf[256];
 
-    bs = bdrv_snapshots();
+    bs = find_vmstate_bs();
     if (!bs) {
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: drop bs_snapshots global variable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On 03/14/2013 10:04 AM, Stefan Hajnoczi wrote:
> The bs_snapshots global variable points to the BlockDriverState which
> will be used to save vmstate.  This is really a savevm.c concept but was
> moved into block.c:bdrv_snapshots() when it became clear that hotplug
> could result in a dangling pointer.
> 
> While auditing the block layer's global state I came upon bs_snapshots
> and realized that a variable is not necessary here.  Simply find the
> first BlockDriverState capable of internal snapshots each time this is
> needed.
> 
> The behavior of bdrv_snapshots() is preserved across hotplug because new
> drives are always appended to the bdrv_states list.  This means that
> calling the new find_vmstate_bs() function is idempotent - it returns
> the same BlockDriverState unless it was hot-unplugged.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block.c               | 28 ----------------------------
>  include/block/block.h |  1 -
>  savevm.c              | 19 +++++++++++++++----
>  3 files changed, 15 insertions(+), 33 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] block: drop bs_snapshots global variable

[Wenchao Xia]

  This fix my problem in moving code to block/snapshot.c.

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

-- 
Best Regards

Wenchao Xia