From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40616 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OTxU2-0002x4-GD for qemu-devel@nongnu.org; Wed, 30 Jun 2010 09:37:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OTxU1-00063a-1q for qemu-devel@nongnu.org; Wed, 30 Jun 2010 09:37:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6441) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OTxU0-00063V-P5 for qemu-devel@nongnu.org; Wed, 30 Jun 2010 09:37:45 -0400 Message-ID: <4C2B4898.8070602@redhat.com> Date: Wed, 30 Jun 2010 15:37:28 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1277484812-22012-1-git-send-email-armbru@redhat.com> <1277484812-22012-10-git-send-email-armbru@redhat.com> In-Reply-To: <1277484812-22012-10-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 09/12] savevm: Survive hot-unplug of snapshot device List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: hch@lst.de, qemu-devel@nongnu.org, kraxel@redhat.com Am 25.06.2010 18:53, schrieb Markus Armbruster: > savevm.c keeps a pointer to the snapshot block device. If you manage > to get that device deleted, the pointer dangles, and the next snapshot > operation will crash & burn. Unplugging a guest device that uses it > does the trick: > > $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...] > QEMU 0.12.50 monitor - type 'help' for more information > (qemu) info snapshots > No available block device supports snapshots > (qemu) drive_add auto if=none,file=tmp.qcow2 > OK > (qemu) device_add usb-storage,id=foo,drive=none1 > (qemu) info snapshots > Snapshot devices: none1 > Snapshot list (from none1): > ID TAG VM SIZE DATE VM CLOCK > (qemu) device_del foo > (qemu) info snapshots > Snapshot devices: > Segmentation fault (core dumped) > > Move management of that pointer to block.c, and zap it when the device > it points to goes away. > > Signed-off-by: Markus Armbruster > --- > block.c | 25 +++++++++++++++++++++++++ > block.h | 1 + > savevm.c | 31 ++++--------------------------- > 3 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/block.c b/block.c > index 5e0ffa0..34055e0 100644 > --- a/block.c > +++ b/block.c > @@ -63,6 +63,9 @@ 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; > > @@ -660,6 +663,9 @@ void bdrv_close_all(void) > void bdrv_delete(BlockDriverState *bs) > { > assert(!bs->peer); > + if (bs == bs_snapshots) { > + bs_snapshots = NULL; > + } This should probably be in bdrv_close() instead. A BlockDriverState can be closed, but not deleted yet; it can't handle snapshots in this state, though. > /* remove from list, if necessary */ > if (bs->device_name[0] != '\0') { > @@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs) > return 1; > } > > +BlockDriverState *bdrv_snapshots(void) > +{ > + BlockDriverState *bs; > + > + if (bs_snapshots) > + return bs_snapshots; I know that this function is just moved with no changes, but while we're at it and you need to respin anyway, can we add braces here? > + > + bs = NULL; > + while ((bs = bdrv_next(bs))) { > + if (bdrv_can_snapshot(bs)) { > + goto ok; > + } > + } > + return NULL; > + ok: > + bs_snapshots = bs; > + return bs; > +} And instead of a goto we could do the right thing directly in the if block. Kevin