From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo3G1-00047g-D5 for qemu-devel@nongnu.org; Tue, 03 Dec 2013 22:36:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo3Fw-000101-IU for qemu-devel@nongnu.org; Tue, 03 Dec 2013 22:36:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46641) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo3Fw-0000zv-AW for qemu-devel@nongnu.org; Tue, 03 Dec 2013 22:36:08 -0500 Message-ID: <529EA317.5060409@redhat.com> Date: Wed, 04 Dec 2013 11:35:51 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1386077165-19577-1-git-send-email-benoit@irqsave.net> <1386077165-19577-7-git-send-email-benoit@irqsave.net> In-Reply-To: <1386077165-19577-7-git-send-email-benoit@irqsave.net> Content-Type: text/plain; charset=UTF-8; format=flowed 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: =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org Cc: kwolf@redhat.com, jcody@redhat.com, armbru@redhat.com, stefanha@redhat.com 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, QE= MUOptionParameter *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 fo= rmat > + * (single child block filter will store their child in bs->file) > + */ > +ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs, > + BlockDriverState *cand= idate) > { > - 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_sna= pshot) { > - 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 *bs= ) > +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs, > + BlockDriverState *cand= idate) > { > + 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 by= going > + * down the forest of bs, skipping filters and stopping on the the fir= st 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 *bs= ); > +ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs, > + BlockDriverState *cand= idate); > +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs, > + BlockDriverState *cand= idate); > +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate); > > /* async block I/O */ > typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t se= ctor, > 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 res= ponse > + /* this table of boolean contains authorizations for the block ope= rations */ > + bool authorizations[BS_AUTHORIZATION_COUNT]; OK, I see some overlap of work here with my image fleecing patch series: http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03694.html The disadvantage of using a bool array is that it can't keep track of=20 multiple points in code that want to forbid the same operation. So I=20 think an array of int is better here. And what do you think of my posted interface, does it works for you? Fam > + /* future complex block filters will implement the following to qu= ery their > + * children to check if snapshoting is allowed on a bs of the grap= h > */ > - ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs); > + ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs, > + BlockDriverState *candi= date); > > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *f= ilename); > int (*bdrv_probe_device)(const char *filename); >