Re: [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen

[Jeff Cody]

On Wed, Apr 05, 2017 at 04:25:39PM -0400, John Snow wrote:
> 
> 
> On 04/05/2017 02:28 PM, Jeff Cody wrote:
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  block.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index ad958b9..3245fae 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2785,6 +2785,7 @@ int bdrv_reopen_prepare(BDRVReopenState 
> > *reopen_state, BlockReopenQueue *queue,
> >      BlockDriver *drv;
> >      QemuOpts *opts;
> >      const char *value;
> > +    bool read_only;
> >  
> >      assert(reopen_state != NULL);
> >      assert(reopen_state->bs->drv != NULL);
> > @@ -2813,12 +2814,13 @@ int bdrv_reopen_prepare(BDRVReopenState 
> > *reopen_state, BlockReopenQueue *queue,
> >          qdict_put(reopen_state->options, "driver", 
> > qstring_from_str(value));
> >      }
> >  
> > -    /* if we are to stay read-only, do not allow permission change
> > -     * to r/w */
> > -    if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
> > -        reopen_state->flags & BDRV_O_RDWR) {
> 
> So the current code checks reopen_state->flags & BDRV_O_RDWR;
> 
> > -        error_setg(errp, "Node '%s' is read only",
> > -                   bdrv_get_device_or_node_name(reopen_state->bs));
> > +    /* If we are to stay read-only, do not allow permission change
> > +     * to r/w. Attempting to set to r/w may fail if either 
> > BDRV_O_ALLOW_RDWR is
> > +     * not set, or if the BDS still has copy_on_read enabled */
> > +    read_only = !(reopen_state->bs->open_flags & BDRV_O_RDWR);
> 
> And the proposed change checks reopen_state->bs->open_flags &
> BDRV_O_RDWR. (It's negated again inside of bdrv_try_set_read_only.)
> 
> Both check against !(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR).
> 
> What's the functional difference of doing so, and is it intentional?
>

Good catch; unintentional.  That line should read:

+    read_only = !(reopen_state->flags & BDRV_O_RDWR);

The functional difference is we would be testing against the open_flags of
the current BS to see if we are requesting r/w or r/o, rather than checking
against the reopen requested flags.

> > +    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> >          goto error;
> >      }
> >  
> >