From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo3R6-0007dC-Os for qemu-devel@nongnu.org; Tue, 03 Dec 2013 22:47:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vo3R0-0004hc-Oa for qemu-devel@nongnu.org; Tue, 03 Dec 2013 22:47:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45055) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vo3R0-0004hX-Gd for qemu-devel@nongnu.org; Tue, 03 Dec 2013 22:47:34 -0500 Message-ID: <529EA5CA.6050608@redhat.com> Date: Wed, 04 Dec 2013 11:47:22 +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]) { 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 *cand= idate) > { > + 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=20 permit, despite of what candidate is (or even it's relevant to bs), then=20 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 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) { this iterates through all the known graph trees (device_list), instead=20 of limiting to only the device that candidate belongs to. Why not just check candidate's permission bitmap and go down from it? If=20 an ancestor need to disable its descendants, it could simply set=20 permission bits of its children and recurse down. Fam