From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56273 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OU1BW-0003GH-VI for qemu-devel@nongnu.org; Wed, 30 Jun 2010 13:34:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OU1BW-0002Kv-23 for qemu-devel@nongnu.org; Wed, 30 Jun 2010 13:34:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:62212) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OU1BV-0002Kk-M3 for qemu-devel@nongnu.org; Wed, 30 Jun 2010 13:34:54 -0400 From: Markus Armbruster References: <1277484812-22012-1-git-send-email-armbru@redhat.com> <1277484812-22012-10-git-send-email-armbru@redhat.com> <4C2B4898.8070602@redhat.com> Date: Wed, 30 Jun 2010 18:34:54 +0200 In-Reply-To: <4C2B4898.8070602@redhat.com> (Kevin Wolf's message of "Wed, 30 Jun 2010 15:37:28 +0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Kevin Wolf Cc: hch@lst.de, qemu-devel@nongnu.org, kraxel@redhat.com Kevin Wolf writes: > 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. Right. I was thinking about the dangling pointer only. My patch works as advertized: it fixes the crash. But zapping bs_snapshots in bdrv_close() is even better. I'll respin. >> /* 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. Separate patch. I hate it when people squash code motion and change together.