[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_s
From: |
Jeff Cody |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10 6/9] block: use bdrv_try_set_read_only() during reopen |
Date: |
Wed, 5 Apr 2017 20:27:34 -0400 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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;
> > }
> >
> >