From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo5hQ-0000sz-QQ for qemu-devel@nongnu.org; Wed, 04 Dec 2013 01:12:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo5hJ-0002Nj-Qs for qemu-devel@nongnu.org; Wed, 04 Dec 2013 01:12:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:15516) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo5hJ-0002Nf-IY for qemu-devel@nongnu.org; Wed, 04 Dec 2013 01:12:33 -0500 Message-ID: <529EC7C3.6040504@redhat.com> Date: Wed, 04 Dec 2013 14:12:19 +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> In-Reply-To: <20131204052006.GB2781@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 13:20, Beno=C3=AEt Canet wrote: > Le Wednesday 04 Dec 2013 =C3=A0 11:47:22 (+0800), Fam Zheng a =C3=A9cri= t : >> 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 *bs= , >>> + BlockDriverState *ca= ndidate) >>> { >>> - 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_s= napshot) { >>> - 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 *ca= ndidate) >>> { >>> + 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 f= irst 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 r= eturn any > spurious success. > But the "candidate =3D=3D bs" check is in bdrv_generic_check_ext_snapshot= ,=20 which gets short-circuited by driver implementation if the driver=20 implements it, in bdrv_recurse_check_ext_snapshot. So if I have an "always yes" drv->bdrv_check_ext_snapshot and it happens=20 to be the first one in bdrv_states, I will allow all snapshot operations. Fam