From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43504 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OKaiB-0008G8-Bm for qemu-devel@nongnu.org; Fri, 04 Jun 2010 13:29:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OKai9-0001f9-Mh for qemu-devel@nongnu.org; Fri, 04 Jun 2010 13:29:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51674) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OKai9-0001ev-8A for qemu-devel@nongnu.org; Fri, 04 Jun 2010 13:29:37 -0400 Message-ID: <4C0937D4.3010607@redhat.com> Date: Fri, 04 Jun 2010 19:28:52 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1275594761-15258-1-git-send-email-miguel.filho@gmail.com> In-Reply-To: <1275594761-15258-1-git-send-email-miguel.filho@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH v3] 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, armbru@redhat.com Am 03.06.2010 21:52, schrieb Miguel Di Ciurcio Filho: > Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as adver= tized. >=20 > First issue: Their names implies different porpouses, but they do the s= ame thing > and have exactly the same code. Maybe copied and pasted and forgotten? > bdrv_has_snapshot() is called in various places for actually checking i= f there > is snapshots or not. >=20 > Second issue: the way bdrv_can_snapshot() verifies if a block driver su= pports or > not snapshots does not catch all cases. E.g.: a raw image. >=20 > 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(). >=20 > static BlockDriverState *get_bs_snapshots(void) > { > BlockDriverState *bs; > DriveInfo *dinfo; >=20 > if (bs_snapshots) > return bs_snapshots; > QTAILQ_FOREACH(dinfo, &drives, next) { > bs =3D dinfo->bdrv; > if (bdrv_can_snapshot(bs)) > goto ok; > } > return NULL; > ok: > bs_snapshots =3D bs; > return bs; > } >=20 > bdrv_can_snapshot() may return a BlockDriverState that does not support > snapshots and do_savevm() goes on. >=20 > Later on in do_savevm(), we find: >=20 > QTAILQ_FOREACH(dinfo, &drives, next) { > bs1 =3D dinfo->bdrv; > if (bdrv_has_snapshot(bs1)) { > /* Write VM state size only to the image that contains the = state */ > sn->vm_state_size =3D (bs =3D=3D bs1 ? vm_state_size : 0); > ret =3D bdrv_snapshot_create(bs1, sn); > if (ret < 0) { > monitor_printf(mon, "Error while creating snapshot on '= %s'\n", > bdrv_get_device_name(bs1)); > } > } > } >=20 > bdrv_has_snapshot(bs1) is not checking if the device does support or ha= s > snapshots as explained above. Only in bdrv_snapshot_create() the device= is > actually checked for snapshot support. >=20 > 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. >=20 > This patch addresses these issues by making bdrv_can_snapshot() actuall= y 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_sn= apshot() > where appropriate. >=20 > bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more s= ense to me. >=20 > The loadvm_state() function was updated too to enforce that when loadin= g a VM at > least all writable devices must support snapshots too. >=20 > Signed-off-by: Miguel Di Ciurcio Filho > --- > block.c | 11 +++++++++++ > block.h | 1 + > savevm.c | 58 ++++++++++++++++++++++++++++++++++++------------------= ---- > 3 files changed, 48 insertions(+), 22 deletions(-) >=20 > diff --git a/block.c b/block.c > index cd70730..ace3cdb 100644 > --- a/block.c > +++ b/block.c > @@ -1720,6 +1720,17 @@ void bdrv_debug_event(BlockDriverState *bs, BlkD= ebugEvent event) > /**************************************************************/ > /* handling of snapshots */ > =20 > +int bdrv_can_snapshot(BlockDriverState *bs) > +{ > + BlockDriver *drv =3D bs->drv; > + if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) || > + bdrv_is_read_only(bs)) { > + return 0; > + } > + > + return 1; > +} > + > int bdrv_snapshot_create(BlockDriverState *bs, > QEMUSnapshotInfo *sn_info) > { > diff --git a/block.h b/block.h > index 24efeb6..fbcd8af 100644 > --- a/block.h > +++ b/block.h > @@ -173,6 +173,7 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriver= Info *bdi); > const char *bdrv_get_encrypted_filename(BlockDriverState *bs); > void bdrv_get_backing_filename(BlockDriverState *bs, > char *filename, int filename_size); > +int bdrv_can_snapshot(BlockDriverState *bs); > int bdrv_snapshot_create(BlockDriverState *bs, > QEMUSnapshotInfo *sn_info); > int bdrv_snapshot_goto(BlockDriverState *bs, > diff --git a/savevm.c b/savevm.c > index dc20390..6549ca7 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1574,22 +1574,6 @@ out: > return ret; > } > =20 > -/* device can contain snapshots */ > -static int bdrv_can_snapshot(BlockDriverState *bs) > -{ > - return (bs && > - !bdrv_is_removable(bs) && > - !bdrv_is_read_only(bs)); > -} > - > -/* device must be snapshots in order to have a reliable snapshot */ > -static int bdrv_has_snapshot(BlockDriverState *bs) > -{ > - return (bs && > - !bdrv_is_removable(bs) && > - !bdrv_is_read_only(bs)); > -} > - > static BlockDriverState *get_bs_snapshots(void) > { > BlockDriverState *bs; > @@ -1599,8 +1583,9 @@ static BlockDriverState *get_bs_snapshots(void) > return bs_snapshots; > QTAILQ_FOREACH(dinfo, &drives, next) { > bs =3D dinfo->bdrv; > - if (bdrv_can_snapshot(bs)) > + if (bdrv_can_snapshot(bs)) { > goto ok; > + } > } > return NULL; > ok: > @@ -1674,12 +1659,26 @@ void do_savevm(Monitor *mon, const QDict *qdict= ) > #endif > const char *name =3D qdict_get_try_str(qdict, "name"); > =20 > + /* Verify if there is a device that doesn't support snapshots and = is writable */ > + QTAILQ_FOREACH(dinfo, &drives, next) { > + bs =3D dinfo->bdrv; > + > + if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) { > + continue; > + } > + > + if (!bdrv_can_snapshot(bs)) { > + monitor_printf(mon, "Device '%s' is writable but does not = support snapshots.\n", > + bdrv_get_device_name(bs)); > + goto the_end; cc1: warnings being treated as errors savevm.c: In Funktion =BBdo_savevm=AB: savevm.c:1654: Fehler: =BBsaved_vm_running=AB may be used uninitialized i= n this function return instead of goto the_end is probably the right thing to do here. Kevin