From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo6V7-0002tv-U2 for qemu-devel@nongnu.org; Wed, 04 Dec 2013 02:04:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo6V1-0007gA-T9 for qemu-devel@nongnu.org; Wed, 04 Dec 2013 02:04:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52388) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo6V1-0007fy-Kz for qemu-devel@nongnu.org; Wed, 04 Dec 2013 02:03:55 -0500 Message-ID: <529ED3CE.1010804@redhat.com> Date: Wed, 04 Dec 2013 15:03:42 +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> <529EA5CA.6050608@redhat.com> <20131204052006.GB2781@irqsave.net> <529EC7C3.6040504@redhat.com> <20131204063409.GE2781@irqsave.net> In-Reply-To: <20131204063409.GE2781@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==?= Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, stefanha@redhat.com 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=A9cri= t : >> 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=A9c= rit : >>>> On 2013=E5=B9=B412=E6=9C=8803=E6=97=A5 21:26, Beno=C3=AEt Canet wrot= e: >>>>> --- >>>>> 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 firs= t 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_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 *= 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 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 authorize= d by going >>>>> + * down the forest of bs, skipping filters and stopping on the the= first bses >>>>> + * authorizing snapshots >>>>> + */ >>>>> +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidat= e) >>>>> +{ >>>>> + 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 on= e (quorum): > > static ExtSnapshotPerm quorum_check_ext_snapshot(BlockDriverState *bs, > BlockDriverState *can= didate) > { > 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], > candida= te); > if (perm =3D=3D EXT_SNAPSHOT_ALLOWED) { > return EXT_SNAPSHOT_ALLOWED; > } > } > > return EXT_SNAPSHOT_FORBIDDEN; > } > > Maybe the callback needs a serious rename. > OK, I see how it works. Default is forbidden and you iterate on all the=20 devices trying to find some BDS recognizes and returns "allow". This=20 positive vote is so powerful and I hope no driver will ever abuse it in=20 the future. :) But I still think if "bs" doesn't "recognize candidate" (in other words,=20 they are irrelevant to each other), it should return a 3rd value like=20 "EXT_SNAPSHOT_NOTCARE", which is more intuitive. Thanks for your explanation. Fam