From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43458) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPBjb-0002IO-He for qemu-devel@nongnu.org; Thu, 26 Sep 2013 09:36:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VPBjW-0004tv-8I for qemu-devel@nongnu.org; Thu, 26 Sep 2013 09:35:59 -0400 Received: from nodalink.pck.nerim.net ([62.212.105.220]:39606 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPBjV-0004tk-U2 for qemu-devel@nongnu.org; Thu, 26 Sep 2013 09:35:54 -0400 Date: Thu, 26 Sep 2013 15:35:49 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20130926133548.GA3338@irqsave.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130926114319.GH2443@dhcp-200-207.str.redhat.com> 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: Kevin Wolf Cc: Jeff Cody , qemu-devel@nongnu.org, stefanha@redhat.com 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 ch= ild. > > > Protocol and drivers designed to be on the bottom of the stack wher= e set to allow > > > snapshots. > > > Future protocols like quorum where creating snapshots does not make= 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 *bs,= 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_forbidd= en > >=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 checks > whether snapshots are forbidden. >=20 > How about bdrv_ext_snapshot_allowed(), which avoid double negations whe= n > 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 Whould .bdrv_check_ext_snapshot being NULL imply "- Do the snapshot here"= as Jeff suggested ? Best regards Beno=EEt >=20 > Kevin