Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> This adds the "read-only" option to the QDict. One important effect of
> this change is that when a child inherits options from its parent, the
> existing "read-only" mode can be preserved if it was explicitly set
> previously.
> 
> This addresses scenarios like this:
> 
>    [E] <- [D] <- [C] <- [B] <- [A]
> 
> In this case, if we reopen [D] with read-only=off, and later reopen
> [B], then [D] will not inherit read-only=on from its parent during the
> bdrv_reopen_queue_child() stage.
> 
> The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the
> value of the "read-only" option.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block.c               | 35 ++++++++++++++++++++++++++++++++---
>  block/vvfat.c         |  3 ++-
>  blockdev.c            | 16 +++++++++++-----
>  include/block/block.h |  1 +
>  4 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f56d703..cf9c513 100644
> --- a/block.c
> +++ b/block.c
> @@ -707,6 +707,9 @@ static void bdrv_inherited_options(int *child_flags, 
> QDict *child_options,
>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
>      qdict_copy_default(child_options, parent_options, 
> BDRV_OPT_CACHE_NO_FLUSH);
>  
> +    /* Inherit the read-only option from the parent if it's not set */
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
> +
>      /* Our block drivers take care to send flushes and respect unmap policy,
>       * so we can default to enable both on lower layers regardless of the
>       * corresponding parent options. */

We need another qdict_copy_default() in bdrv_temp_snapshot_options(), I
think, so that flags and options stay consistent there.

Currently it seems to be working because we still have the flag and the
flag is what is actually used in bdrv_open_common(). This means that we
parse the "read-only" option into the QemuOpts there, but we never read
it from there. Shouldn't we use the option rather than the flags?

> @@ -677,11 +679,15 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, 
> Error **errp)
>          goto fail;
>      }
>  
> +    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
> +
>      /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
>       * with other callers) rather than what we want as the real defaults.
>       * Apply the defaults here instead. */
>      qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
>      qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
> +    qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
> +                          read_only ? "on" : "off");

Why do you parse read_only into the QemuOpts just to add it right back
to bs_opts? Wouldn't it be easier to remove it from qemu_root_bds_opts
and do a simple set_default_str("on") here? (Which is how the cache
options work.)

Kevin