On Wed, May 29, 2013 at 03:54:46PM +0800, Wenchao Xia wrote: > δΊ 2013-5-28 15:46, Stefan Hajnoczi ει: > >On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote: > >>Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben: > >>>From: Stefan Hajnoczi <stefa...@redhat.com> > >>> > >>>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 <stefa...@redhat.com> > >>>Reviewed-by: Eric Blake <ebl...@redhat.com> > >>>Reviewed-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com> > >>>Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.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 3f87489..478a3b2 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; > >>> > >>>@@ -1357,9 +1354,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; > >>>@@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs) > >>> > >>> bdrv_close(bs); > >>> > >>>- assert(bs != bs_snapshots); > >>> g_free(bs); > >>> } > >>> > >>>@@ -1635,9 +1628,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; > >>>- } > >> > >>This hunk isn't replaced by any other code. If I understand correctly > >>what it's doing, it prevented you from saving the VM state to a > >>removable device, which would be allowed after this patch. > >> > >>Is this a change we want to make? Why isn't it described in the commit > >>message? > > > >My understanding of this change is different. Markus is on CC so maybe > >he can confirm. > > > >The point of bs_snapshots = NULL is not to prevent you from saving > >snapshots. It's simply to reset the pointer to the next snapshottable > >device (used by bdrv_snapshots()). > > > >See the bdrv_close() hunk above which does the same thing, as well as > >bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots. > > > >So what this hunk does is to reset the bdrv_snapshots() iterator when a > >removable device is hooked up to an emulated storage controller. It's > >no longer necessary since this patch drops the global state > >(bs_snapshots) and users will always iterate from scratch. > > > >The whole stateful approach was not necessary. > > > >Stefan > > > Reading the code, original logic actually forbidded saving vmstate > into a removable device, now it is possible since find_vmstate_bs() > doesn't check it. How about forbid again in find_vmstate_bs()?
I don't follow. The hunk that Kevin quoted ensures that we restart bs_snapshots iteration - this prevents us from choosing a device that has no medium inserted (also see bdrv_can_snapshot() which checks for an inserted medium). This behavior is preserved in this patch because we now always restart iteration and it's no longer necessary to reset global iterator state. But I don't see any code that forbids/skips inserted, read-write removable devices in the orginal code. Please point out the specific piece of code you think has been dropped. Stefan