diff mbox

[for-2.10,6/9] block: use bdrv_try_set_read_only() during reopen

Message ID abc6320f80ff4af4788fe33b12bb12cbfa83d980.1491416061.git.jcody@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Cody April 5, 2017, 6:28 p.m. UTC 
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

John Snow April 5, 2017, 8:25 p.m. UTC | #1 
On 04/05/2017 02:28 PM, Jeff Cody wrote:
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  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?

> +    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          goto error;
>      }
>  
>
Jeff Cody April 6, 2017, 12:27 a.m. UTC | #2 
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 <jcody@redhat.com>
> > ---
> >  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;
> >      }
> >  
> >
diff mbox

Patch

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) {
-        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);
+    ret = bdrv_try_set_read_only(reopen_state->bs, read_only, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         goto error;
     }