Re: [Qemu-devel] [PATCH] block: Add bdrv_forbid_ext_snapshots.

[Benoît Canet]

Le Thursday 26 Sep 2013 à 13:43:19 (+0200), Kevin Wolf a écrit :
> Am 26.09.2013 um 04:01 hat Jeff Cody geschrieben:
> > On Wed, Sep 25, 2013 at 04:23:22PM +0200, Benoît 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 where set 
> > > to allow
> > > snapshots.
> > > Future protocols like quorum where creating snapshots does not make sense
> > > without block filters will be set to forbid snapshots.
> > > 
> > > Signed-off-by: Benoit Canet <address@hidden>
> 
> > > 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)
> > > +{
> > 
> > I think either:
> > A) Name this function bdrv_forbid_ext_snapshots(), or
> > B) Name the BlockDriver function ptr to .bdrv_is_ext_snapshot_forbidden
> > 
> > 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

Whould .bdrv_check_ext_snapshot being NULL imply "- Do the snapshot here" as
Jeff suggested ?

Best regards

Benoît

> 
> Kevin