From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57144 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OLYqi-0000qw-VP for qemu-devel@nongnu.org; Mon, 07 Jun 2010 05:42:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OLYqe-00046j-OB for qemu-devel@nongnu.org; Mon, 07 Jun 2010 05:42:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10828) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OLYqe-00046b-Gk for qemu-devel@nongnu.org; Mon, 07 Jun 2010 05:42:24 -0400 Message-ID: <4C0CBEEE.1050201@redhat.com> Date: Mon, 07 Jun 2010 11:42:06 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1275680159-30396-1-git-send-email-miguel.filho@gmail.com> In-Reply-To: <1275680159-30396-1-git-send-email-miguel.filho@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v4] savevm: Really verify if a drive supports snapshots List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Miguel Di Ciurcio Filho Cc: qemu-devel@nongnu.org Am 04.06.2010 21:35, schrieb Miguel Di Ciurcio Filho: > Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized. > > First issue: Their names implies different porpouses, but they do the same thing > and have exactly the same code. Maybe copied and pasted and forgotten? > bdrv_has_snapshot() is called in various places for actually checking if there > is snapshots or not. > > Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or > not snapshots does not catch all cases. E.g.: a raw image. > > So when do_savevm() is called, first thing it does is to set a global > BlockDriverState to save the VM memory state calling get_bs_snapshots(). > > static BlockDriverState *get_bs_snapshots(void) > { > BlockDriverState *bs; > DriveInfo *dinfo; > > if (bs_snapshots) > return bs_snapshots; > QTAILQ_FOREACH(dinfo, &drives, next) { > bs = dinfo->bdrv; > if (bdrv_can_snapshot(bs)) > goto ok; > } > return NULL; > ok: > bs_snapshots = bs; > return bs; > } > > bdrv_can_snapshot() may return a BlockDriverState that does not support > snapshots and do_savevm() goes on. > > Later on in do_savevm(), we find: > > QTAILQ_FOREACH(dinfo, &drives, next) { > bs1 = dinfo->bdrv; > if (bdrv_has_snapshot(bs1)) { > /* Write VM state size only to the image that contains the state */ > sn->vm_state_size = (bs == bs1 ? vm_state_size : 0); > ret = bdrv_snapshot_create(bs1, sn); > if (ret < 0) { > monitor_printf(mon, "Error while creating snapshot on '%s'\n", > bdrv_get_device_name(bs1)); > } > } > } > > bdrv_has_snapshot(bs1) is not checking if the device does support or has > snapshots as explained above. Only in bdrv_snapshot_create() the device is > actually checked for snapshot support. > > So, in cases where the first device supports snapshots, and the second does not, > the snapshot on the first will happen anyways. I believe this is not a good > behavior. It should be an all or nothing process. > > This patch addresses these issues by making bdrv_can_snapshot() actually do > what it must do and enforces better tests to avoid errors in the middle of > do_savevm(). bdrv_has_snapshot() is removed and replaced by bdrv_can_snapshot() > where appropriate. > > bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense to me. > > The loadvm_state() function was updated too to enforce that when loading a VM at > least all writable devices must support snapshots too. > > Signed-off-by: Miguel Di Ciurcio Filho Thanks, applied to the block branch. Kevin