From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50574) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPCCi-0002pk-Uf for qemu-devel@nongnu.org; Thu, 26 Sep 2013 10:06:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VPCCd-0005sD-Tt for qemu-devel@nongnu.org; Thu, 26 Sep 2013 10:06:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39073) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPCCd-0005s7-Kk for qemu-devel@nongnu.org; Thu, 26 Sep 2013 10:05:59 -0400 Date: Thu, 26 Sep 2013 16:05:50 +0200 From: Kevin Wolf Message-ID: <20130926140550.GK2443@dhcp-200-207.str.redhat.com> References: <1380119002-18953-1-git-send-email-benoit@irqsave.net> <1380119002-18953-2-git-send-email-benoit@irqsave.net> <20130926020107.GB5181@localhost.localdomain> <20130926114319.GH2443@dhcp-200-207.str.redhat.com> <20130926133548.GA3338@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130926133548.GA3338@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: Jeff Cody , qemu-devel@nongnu.org, stefanha@redhat.com Am 26.09.2013 um 15:35 hat Beno=EEt Canet geschrieben: > Le Thursday 26 Sep 2013 =E0 13:43:19 (+0200), Kevin Wolf a =E9crit : > > Am 26.09.2013 um 04:01 hat Jeff Cody geschrieben: > > > On Wed, Sep 25, 2013 at 04:23:22PM +0200, Beno=EEt Canet wrote: > > > > Drivers having a bs->file where set to recurse the call to their = child. > > > > Protocol and drivers designed to be on the bottom of the stack wh= ere set to allow > > > > snapshots. > > > > Future protocols like quorum where creating snapshots does not ma= ke sense > > > > without block filters will be set to forbid snapshots. > > > >=20 > > > > Signed-off-by: Benoit Canet > >=20 > > > > diff --git a/block.c b/block.c > > > > index 4a98250..ff296df 100644 > > > > --- a/block.c > > > > +++ b/block.c > > > > @@ -4651,3 +4651,30 @@ int bdrv_amend_options(BlockDriverState *b= s, QEMUOptionParameter *options) > > > > } > > > > return bs->drv->bdrv_amend_options(bs, options); > > > > } > > > > + > > > > +bool bdrv_is_ext_snapshot_forbidden(BlockDriverState *bs) > > > > +{ > > >=20 > > > I think either: > > > A) Name this function bdrv_forbid_ext_snapshots(), or > > > B) Name the BlockDriver function ptr to .bdrv_is_ext_snapshot_forbi= dden > > >=20 > > > The idea being that this function and the BlockDriver function ptr > > > should have the same name (e.g. bdrv_has_zero_init, and > > > bs->drv->bdrv_has_zero_init, etc..) > >=20 > > Yes, I agree, some consistent naming is desirable. I don't think > > bdrv_forbid_ext_snapshots() is a good name, because it implies that > > calling this function is what forbids the snapshot (i.e. an action > > similar to adding a migration blocker), whereas in fact it just check= s > > whether snapshots are forbidden. > >=20 > > How about bdrv_ext_snapshot_allowed(), which avoid double negations w= hen > > we check for "not forbidden"? Or perhaps even bdrv_check_ext_snapshot= (), > > which would be a more generic name that could be extended to the > > three-way distinction we intended to have in the end: > >=20 > > - External snapshots are forbidden > > - May snapshot, but below this BDS (ask bs->file; this is for filters= ) > > - Do the snapshot here >=20 > Whould .bdrv_check_ext_snapshot being NULL imply "- Do the snapshot her= e" as > Jeff suggested ? That would probably be the most convenient option. Kevin