Re: [Qemu-devel] [PATCH for-2.10 5/9] block: introduce bdrv_try_set_read_only()

[Jeff Cody <jcody@redhat.com>]

On Wed, Apr 05, 2017 at 04:28:44PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > Introduce try function for setting read_only flags.  Will return < 0 on
> > error, with appropriate Error value set.  Does not alter any flags.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 14 +++++++++++++-
> >  include/block/block.h |  1 +
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 8bfe7f4..ad958b9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -197,7 +197,7 @@ bool bdrv_is_read_only(BlockDriverState *bs)
> >      return bs->read_only;
> >  }
> >  
> > -int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >  {
> >      /* Do not set read_only if copy_on_read is enabled */
> >      if (bs->copy_on_read && read_only) {
> > @@ -213,6 +213,18 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> >          return -EPERM;
> >      }
> >  
> > +    return 0;
> > +}
> > +
> > +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
> > +{
> > +    int ret = 0;
> > +
> > +    ret = bdrv_try_set_read_only(bs, read_only, errp);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> >      bs->read_only = read_only;
> >      return 0;
> >  }
> > diff --git a/include/block/block.h b/include/block/block.h
> > index beb563a..0049b57 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -426,6 +426,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> >                              int64_t sector_num, int nb_sectors, int *pnum);
> >  
> >  bool bdrv_is_read_only(BlockDriverState *bs);
> > +int bdrv_try_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
> >  bool bdrv_is_sg(BlockDriverState *bs);
> >  bool bdrv_is_inserted(BlockDriverState *bs);
> > 
> 
> Maybe I'm just green, but I find it weird that the function is named
> try_x when we aren't actually trying to do anything, we're checking to
> see if we can.

Yeah, not a good name.  Maybe bdrv_set_read_only_test() or
bdrv_test_set_read_only()?