From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48562) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VP9ym-0002iT-P5 for qemu-devel@nongnu.org; Thu, 26 Sep 2013 07:43:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VP9yh-0005LM-K0 for qemu-devel@nongnu.org; Thu, 26 Sep 2013 07:43:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39764) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VP9yh-0005LG-9h for qemu-devel@nongnu.org; Thu, 26 Sep 2013 07:43:27 -0400 Date: Thu, 26 Sep 2013 13:43:19 +0200 From: Kevin Wolf Message-ID: <20130926114319.GH2443@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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130926020107.GB5181@localhost.localdomain> 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: Jeff Cody Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , stefanha@redhat.com, qemu-devel@nongnu.org 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 chil= d. > > Protocol and drivers designed to be on the bottom of the stack where = set to allow > > snapshots. > > Future protocols like quorum where creating snapshots does not make s= ense > > without block filters will be set to forbid snapshots. > >=20 > > Signed-off-by: Benoit Canet > > 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, Q= EMUOptionParameter *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_forbidden >=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..) 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. How about bdrv_ext_snapshot_allowed(), which avoid double negations when 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: - External snapshots are forbidden - May snapshot, but below this BDS (ask bs->file; this is for filters) - Do the snapshot here Kevin