From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51367) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhEcY-0004PQ-II for qemu-devel@nongnu.org; Tue, 28 May 2013 03:47:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UhEcQ-0006G3-U3 for qemu-devel@nongnu.org; Tue, 28 May 2013 03:47:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28263) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UhEcQ-0006Fw-Lk for qemu-devel@nongnu.org; Tue, 28 May 2013 03:46:54 -0400 Date: Tue, 28 May 2013 09:46:49 +0200 From: Stefan Hajnoczi Message-ID: <20130528074649.GC13368@stefanha-thinkpad.redhat.com> References: <1369451385-23452-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1369451385-23452-2-git-send-email-xiawenc@linux.vnet.ibm.com> <20130527152516.GF2373@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130527152516.GF2373@dhcp-200-207.str.redhat.com> Subject: Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: phrdina@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com, Wenchao Xia , armbru@redhat.com 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 > > > > 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 > > Reviewed-by: Eric Blake > > Reviewed-by: Wenchao Xia > > Signed-off-by: Wenchao Xia > > --- > > 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