From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo62a-0001WJ-GY for qemu-devel@nongnu.org; Wed, 04 Dec 2013 01:34:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo62U-00082t-IR for qemu-devel@nongnu.org; Wed, 04 Dec 2013 01:34:32 -0500 Received: from nodalink.pck.nerim.net ([62.212.105.220]:43682 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo62T-00082g-WD for qemu-devel@nongnu.org; Wed, 04 Dec 2013 01:34:26 -0500 Date: Wed, 4 Dec 2013 07:34:10 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20131204063409.GE2781@irqsave.net> References: <1386077165-19577-1-git-send-email-benoit@irqsave.net> <1386077165-19577-7-git-send-email-benoit@irqsave.net> <529EA5CA.6050608@redhat.com> <20131204052006.GB2781@irqsave.net> <529EC7C3.6040504@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <529EC7C3.6040504@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: =?iso-8859-1?Q?Beno=EEt?= Canet , kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, stefanha@redhat.com Le Wednesday 04 Dec 2013 =C3=A0 14:12:19 (+0800), Fam Zheng a =C3=A9crit = : > On 2013=E5=B9=B412=E6=9C=8804=E6=97=A5 13:20, Beno=C3=AEt Canet wrote: > >Le Wednesday 04 Dec 2013 =C3=A0 11:47:22 (+0800), Fam Zheng a =C3=A9cr= it : > >>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,= QEMUOptionParameter *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= format > >>>+ * (single child block filter will store their child in bs->file) > >>>+ */ > >>>+ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *b= s, > >>>+ BlockDriverState *c= andidate) > >>> { > >>>- 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_= snapshot) { > >>>- return bs->file->drv->bdrv_check_ext_snapshot(bs); > >>>+ if (!bs->drv->authorizations[BS_CANT_SNAPSHOT]) { > >> > >>This double negative feels hard to read for me. > >> > >>>+ 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 *b= s, > >>>+ BlockDriverState *c= andidate) > >>> { > >>>+ if (bs->drv && bs->drv->bdrv_check_ext_snapshot) { > >>>+ return bs->drv->bdrv_check_ext_snapshot(bs, candidate); > >>>+ } > >> > >>Maybe I'm missing something, but if a driver always returns positive > >>permit, despite of what candidate is (or even it's relevant to bs), > >>then doesn't it also affect other devices? because... > >> > >>>+ > >>>+ 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 = first 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) { > >> > >>this iterates through all the known graph trees (device_list), > >>instead of limiting to only the device that candidate belongs to. > > > >The recursion termination success is candidate =3D=3D bs. > >This make sure that the scan of the other tree of the forest will not = return any > >spurious success. > > > > But the "candidate =3D=3D bs" check is in > bdrv_generic_check_ext_snapshot, which gets short-circuited by > driver implementation if the driver implements it, in > bdrv_recurse_check_ext_snapshot. > > So if I have an "always yes" drv->bdrv_check_ext_snapshot and it > happens to be the first one in bdrv_states, I will allow all > snapshot operations. > My bad I forgot to document the drv_>bdrv_check_ext_snapshot. It meant to be recursive and only for twisted block filter like this one = (quorum): static ExtSnapshotPerm quorum_check_ext_snapshot(BlockDriverState *bs, BlockDriverState *candid= ate) { BDRVQuorumState *s =3D bs->opaque; int i; for (i =3D 0; i < s->total; i++) { ExtSnapshotPerm perm =3D bdrv_recurse_check_ext_snapshot(s->bs[i]= , candidate)= ; if (perm =3D=3D EXT_SNAPSHOT_ALLOWED) { return EXT_SNAPSHOT_ALLOWED; } } return EXT_SNAPSHOT_FORBIDDEN; } Maybe the callback needs a serious rename. Best regards Beno=C3=AEt > Fam