[for-2.10,5/9] block: introduce bdrv_try_set_read_only()

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(-)

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.

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()?

On Wed, Apr 05, 2017 at 08:23:12PM -0400, Jeff Cody wrote:
> On Wed, Apr 05, 2017 at 04:28:44PM -0400, John Snow wrote:
> > On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > > 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()?

This one reads nicely:

  if (bdrv_can_set_read_only(...)) {
      ...
  }