From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55023) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo4vd-0005Fe-M4 for qemu-devel@nongnu.org; Wed, 04 Dec 2013 00:23:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo4vY-0005VD-RE for qemu-devel@nongnu.org; Wed, 04 Dec 2013 00:23:17 -0500 Received: from nodalink.pck.nerim.net ([62.212.105.220]:43635 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo4vY-0005Tv-9U for qemu-devel@nongnu.org; Wed, 04 Dec 2013 00:23:12 -0500 Date: Wed, 4 Dec 2013 06:22:59 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20131204052258.GC2781@irqsave.net> References: <1386077165-19577-1-git-send-email-benoit@irqsave.net> <1386077165-19577-7-git-send-email-benoit@irqsave.net> <529EA317.5060409@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <529EA317.5060409@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com Le Wednesday 04 Dec 2013 =C3=A0 11:35:51 (+0800), Fam Zheng a =C3=A9crit = : > On 2013=E5=B9=B412=E6=9C=8803=E6=97=A5 21:26, Beno=C3=AEt Canet wrote: > >--- > > block.c | 64 +++++++++++++++++++++++++++++++++++++= ++++------ > > block/blkverify.c | 2 +- > > include/block/block.h | 16 +++++++++--- > > include/block/block_int.h | 9 ++++--- > > 4 files changed, 75 insertions(+), 16 deletions(-) > > > >diff --git a/block.c b/block.c > >index 8016ff2..0569cb2 100644 > >--- a/block.c > >+++ b/block.c > >@@ -4945,21 +4945,69 @@ int bdrv_amend_options(BlockDriverState *bs, Q= EMUOptionParameter *options) > > return bs->drv->bdrv_amend_options(bs, options); > > } > > > >-ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs) > >+/* will be used to recurse on single child block filter until first f= ormat > >+ * (single child block filter will store their child in bs->file) > >+ */ > >+ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs, > >+ BlockDriverState *can= didate) > > { > >- if (bs->drv->bdrv_check_ext_snapshot) { > >- return bs->drv->bdrv_check_ext_snapshot(bs); > >+ if (!bs->drv) { > >+ return EXT_SNAPSHOT_FORBIDDEN; > > } > > > >- if (bs->file && bs->file->drv && bs->file->drv->bdrv_check_ext_sn= apshot) { > >- return bs->file->drv->bdrv_check_ext_snapshot(bs); > >+ if (!bs->drv->authorizations[BS_CANT_SNAPSHOT]) { > >+ if (bs =3D=3D candidate) { > >+ return EXT_SNAPSHOT_ALLOWED; > >+ } else { > >+ return EXT_SNAPSHOT_FORBIDDEN; > >+ } > > } > > > >- /* external snapshots are allowed by default */ > >- return EXT_SNAPSHOT_ALLOWED; > >+ if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) { > >+ return EXT_SNAPSHOT_FORBIDDEN; > >+ } > >+ > >+ if (!bs->file) { > >+ return EXT_SNAPSHOT_FORBIDDEN; > >+ } > >+ > >+ return bdrv_recurse_check_ext_snapshot(bs->file, candidate); > > } > > > >-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *b= s) > >+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs, > >+ BlockDriverState *can= didate) > > { > >+ if (bs->drv && bs->drv->bdrv_check_ext_snapshot) { > >+ return bs->drv->bdrv_check_ext_snapshot(bs, candidate); > >+ } > >+ > >+ return bdrv_generic_check_ext_snapshot(bs, candidate); > >+} > >+ > >+/* This function check if the candidate bs has snapshots authorized b= y going > >+ * down the forest of bs, skipping filters and stopping on the the fi= rst bses > >+ * authorizing snapshots > >+ */ > >+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate) > >+{ > >+ BlockDriverState *bs; > >+ > >+ /* walk down the bs forest recursively */ > >+ QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > >+ ExtSnapshotPerm perm; > >+ > >+ if (!bs->file) { > >+ continue; > >+ } > >+ > >+ perm =3D bdrv_recurse_check_ext_snapshot(bs->file, candidate)= ; > >+ > >+ /* allowed in the right subtree -> stop here */ > >+ if (perm =3D=3D EXT_SNAPSHOT_ALLOWED) { > >+ return EXT_SNAPSHOT_ALLOWED; > >+ } > >+ } > >+ > >+ /* external snapshots are forbidden by default */ > > return EXT_SNAPSHOT_FORBIDDEN; > > } > >diff --git a/block/blkverify.c b/block/blkverify.c > >index e755e4e..b93017c 100644 > >--- a/block/blkverify.c > >+++ b/block/blkverify.c > >@@ -313,7 +313,7 @@ static BlockDriver bdrv_blkverify =3D { > > .bdrv_aio_writev =3D blkverify_aio_writev, > > .bdrv_aio_flush =3D blkverify_aio_flush, > > > >- .bdrv_check_ext_snapshot =3D bdrv_check_ext_snapshot_forbidden, > >+ .authorizations =3D { true, false }, > > }; > > > > static void bdrv_blkverify_init(void) > >diff --git a/include/block/block.h b/include/block/block.h > >index 26c48e7..73c59fe 100644 > >--- a/include/block/block.h > >+++ b/include/block/block.h > >@@ -280,16 +280,24 @@ int bdrv_amend_options(BlockDriverState *bs_new,= QEMUOptionParameter *options); > > /* external snapshots */ > > > > typedef enum { > >- EXT_SNAPSHOT_ALLOWED, > > EXT_SNAPSHOT_FORBIDDEN, > >+ EXT_SNAPSHOT_ALLOWED, > > } ExtSnapshotPerm; > > > >+typedef enum { > >+ BS_CANT_SNAPSHOT, > >+ BS_FILTER_PASS_DOWN, > >+ BS_AUTHORIZATION_COUNT, > >+} BsAuthorization; > >+ > > /* return EXT_SNAPSHOT_ALLOWED if external snapshot is allowed > > * return EXT_SNAPSHOT_FORBIDDEN if external snapshot is forbidden > > */ > >-ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs); > >-/* helper used to forbid external snapshots like in blkverify */ > >-ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *b= s); > >+ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs, > >+ BlockDriverState *can= didate); > >+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs, > >+ BlockDriverState *can= didate); > >+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate); > > > > /* async block I/O */ > > typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t s= ector, > >diff --git a/include/block/block_int.h b/include/block/block_int.h > >index 9e789d2..d9704f2 100644 > >--- a/include/block/block_int.h > >+++ b/include/block/block_int.h > >@@ -69,10 +69,13 @@ struct BlockDriver { > > const char *format_name; > > int instance_size; > > > >- /* if not defined external snapshots are allowed > >- * future block filters will query their children to build the re= sponse > >+ /* this table of boolean contains authorizations for the block op= erations */ > >+ bool authorizations[BS_AUTHORIZATION_COUNT]; >=20 > OK, I see some overlap of work here with my image fleecing patch series= : >=20 > http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03694.html >=20 > The disadvantage of using a bool array is that it can't keep track > of multiple points in code that want to forbid the same operation. > So I think an array of int is better here. >=20 > And what do you think of my posted interface, does it works for you? Your is in BlockDriverState mine is in BlockDriver so there are not compa= tible. I think we need to get advice from other people. Best regards Beno=C3=AEt >=20 > Fam >=20 > >+ /* future complex block filters will implement the following to q= uery their > >+ * children to check if snapshoting is allowed on a bs of the gra= ph > > */ > >- ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs); > >+ ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs, > >+ BlockDriverState *cand= idate); > > > > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *= filename); > > int (*bdrv_probe_device)(const char *filename); > > >=20 >=20