From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44111) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCwnf-0005Gc-LP for qemu-devel@nongnu.org; Mon, 10 Feb 2014 14:45:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCwna-0001YH-75 for qemu-devel@nongnu.org; Mon, 10 Feb 2014 14:45:51 -0500 Received: from paradis.irqsave.net ([62.212.105.220]:49612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCwnZ-0001Xh-ED for qemu-devel@nongnu.org; Mon, 10 Feb 2014 14:45:46 -0500 Date: Mon, 10 Feb 2014 20:45:45 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140210194544.GD5205@irqsave.net> References: <1390509099-695-1-git-send-email-benoit.canet@irqsave.net> <1390509099-695-6-git-send-email-benoit.canet@irqsave.net> <20140204001503.GA29994@localhost.localdomain> <20140204102551.GD3384@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140204102551.GD3384@dhcp-200-207.str.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V6 5/8] block: Create authorizations mechanism for external snapshot and resize. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , famz@redhat.com, armbru@redhat.com, Jeff Cody , qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com Le Tuesday 04 Feb 2014 =E0 11:25:52 (+0100), Kevin Wolf a =E9crit : > Am 04.02.2014 um 01:15 hat Jeff Cody geschrieben: > > On Thu, Jan 23, 2014 at 09:31:36PM +0100, Beno=EEt Canet wrote: > > > From: Beno=EEt Canet > > >=20 > > > Signed-off-by: Benoit Canet > > > --- > > > block.c | 65 +++++++++++++++++++++++++++++++++++= +++++------- > > > block/blkverify.c | 2 +- > > > blockdev.c | 2 +- > > > include/block/block.h | 20 +++++++-------- > > > include/block/block_int.h | 12 ++++++--- > > > 5 files changed, 77 insertions(+), 24 deletions(-) > > >=20 > > > diff --git a/block.c b/block.c > > > index e1bc732..3e0994b 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -5088,21 +5088,68 @@ int bdrv_amend_options(BlockDriverState *bs= , QEMUOptionParameter *options) > > > return bs->drv->bdrv_amend_options(bs, options); > > > } > > > =20 > > > -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs) > > > +/* Used to recurse on single child block filters. > > > + * Single child block filter will store their child in bs->file. > > > + */ > > > +bool bdrv_generic_is_first_non_filter(BlockDriverState *bs, > > > + BlockDriverState *candidate) > > > { > > > - if (bs->drv->bdrv_check_ext_snapshot) { > > > - return bs->drv->bdrv_check_ext_snapshot(bs); > > > + if (!bs->drv) { > > > + return false; > > > + } > > > + > > > + if (!bs->drv->authorizations[BS_IS_A_FILTER]) { > > > + if (bs =3D=3D candidate) { > > > + return true; > > > + } else { > > > + return false; > > > + } > >=20 > > This seems to break external snapshots; after this patch, I can no > > longer perform live ext snapshots (on qcow2, raw, etc..), unless I am > > doing something incorrectly. > >=20 > > Instead of checking for bs =3D=3D candidate, was it intended to check= to > > see if !strcmp(bs->filename, candidiate->filename) was true? >=20 > If believe the problem is in bdrv_is_first_non_filter(): It starts with > bs->file, whereas the first non-filter is obviously the top-level BDS > itself. I suspect the following patch fixes it (it makes the simple > snapshotting case work again, but I'm not sure if it forbids everything > that should be forbidden). >=20 > In any case, this shows that... >=20 > - ...our testing is still lacking (no qemu-iotests case for live > snapshots? Seriously? Expect it to be broken then.) >=20 > - ...not all patch authors do a good share of manual testing >=20 > - ...I have relaxed my reviewing too much. I wasn't convinced that this > patch is right, because the whole logic confused me, but I couldn't > point to a bug. I shouldn't have merged it when in doubt. >=20 > Jeff, would you like to submit a qemu-iotests case for snapshotting? >=20 > Beno=EEt, can you check whether the patch below is correct? >=20 > Kevin This patch is not correct I will dig the issue. Best regards Beno=EEt feel free to ping me on irc when you fell I miss an email. >=20 >=20 > diff --git a/block.c b/block.c > index ac0ccac..1299484 100644 > --- a/block.c > +++ b/block.c > @@ -5412,11 +5412,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *= candidate) > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > bool perm; > =20 > - if (!bs->file) { > - continue; > - } > - > - perm =3D bdrv_recurse_is_first_non_filter(bs->file, candidate)= ; > + perm =3D bdrv_recurse_is_first_non_filter(bs, candidate); > =20 > /* candidate is the first non filter */ > if (perm) { >=20 >=20