[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict |
Date: |
Thu, 15 Sep 2016 12:51:27 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
- [Qemu-devel] [PATCH 1/7] block: Remove bdrv_is_snapshot, (continued)