From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49456) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoaIi-0007Hz-ML for qemu-devel@nongnu.org; Thu, 05 Dec 2013 09:53:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VoaIc-0006Gr-A9 for qemu-devel@nongnu.org; Thu, 05 Dec 2013 09:53:12 -0500 Received: from nodalink.pck.nerim.net ([62.212.105.220]:44731 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VoaIb-0006Gm-Py for qemu-devel@nongnu.org; Thu, 05 Dec 2013 09:53:06 -0500 Date: Thu, 5 Dec 2013 15:52:56 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20131205145256.GF2892@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> <20131204063409.GE2781@irqsave.net> <529ED3CE.1010804@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <529ED3CE.1010804@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 15:03:42 (+0800), Fam Zheng a =C3=A9crit = : > On 2013=E5=B9=B412=E6=9C=8804=E6=97=A5 14:34, Beno=C3=AEt Canet wrote: > >Le Wednesday 04 Dec 2013 =C3=A0 14:12:19 (+0800), Fam Zheng a =C3=A9cr= it : > >>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=A9= crit : > >>>>On 2013=E5=B9=B412=E6=9C=8803=E6=97=A5 21:26, Beno=C3=AEt Canet wro= te: > >>>>>--- > >>>>> 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 *b= s, 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 fir= st format > >>>>>+ * (single child block filter will store their child in bs->file) > >>>>>+ */ > >>>>>+ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState = *bs, > >>>>>+ BlockDriverState = *candidate) > >>>>> { > >>>>>- 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_ex= t_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(BlockDriverStat= e *bs) > >>>>>+ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState = *bs, > >>>>>+ BlockDriverState = *candidate) > >>>>> { > >>>>>+ 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 positiv= e > >>>>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 authoriz= ed by going > >>>>>+ * down the forest of bs, skipping filters and stopping on the th= e first bses > >>>>>+ * authorizing snapshots > >>>>>+ */ > >>>>>+ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candida= te) > >>>>>+{ > >>>>>+ 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 no= t 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 o= ne (quorum): > > > >static ExtSnapshotPerm quorum_check_ext_snapshot(BlockDriverState *bs, > > BlockDriverState *ca= ndidate) > >{ > > BDRVQuorumState *s =3D bs->opaque; > > int i; > > > > for (i =3D 0; i < s->total; i++) { > > ExtSnapshotPerm perm =3D bdrv_recurse_check_ext_snapshot(s->b= s[i], > > candid= ate); > > if (perm =3D=3D EXT_SNAPSHOT_ALLOWED) { > > return EXT_SNAPSHOT_ALLOWED; > > } > > } > > > > return EXT_SNAPSHOT_FORBIDDEN; > >} > > > >Maybe the callback needs a serious rename. > > >=20 > OK, I see how it works. Default is forbidden and you iterate on all > the devices trying to find some BDS recognizes and returns "allow". > This positive vote is so powerful and I hope no driver will ever > abuse it in the future. :) I will add some explanations to the code to make it clearer. >=20 > But I still think if "bs" doesn't "recognize candidate" (in other > words, they are irrelevant to each other), it should return a 3rd > value like "EXT_SNAPSHOT_NOTCARE", which is more intuitive. Good idea I will do this. >=20 > Thanks for your explanation. >=20 > Fam