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

[John Snow]

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?

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