diff mbox

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

Message ID af68323e39cb61bc54d42da8001466906dc855fc.1473867967.git.berto@igalia.com
State New
Headers show

Commit Message

Alberto Garcia Sept. 14, 2016, 3:52 p.m. UTC
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 <berto@igalia.com>
---
 block.c               | 35 ++++++++++++++++++++++++++++++++---
 block/vvfat.c         |  3 ++-
 blockdev.c            | 16 +++++++++++-----
 include/block/block.h |  1 +
 4 files changed, 46 insertions(+), 9 deletions(-)

Comments

Kevin Wolf Sept. 15, 2016, 10:51 a.m. UTC | #1
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 <berto@igalia.com>
> ---
>  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
Alberto Garcia Sept. 15, 2016, 11:42 a.m. UTC | #2
On Thu 15 Sep 2016 12:51:27 PM CEST, Kevin Wolf wrote:
>> @@ -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.

You're right, I assumed that with read-only=on,snapshot=on the temporary
file would still be r/w.

I'll fix it.

>> +    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.)

Yeah, I was mirroring blockdev_init() (there it's easier to keep it in
qemu_common_drive_opts), but in this case there's no need to do it.

I'll fix it too.

Thanks!

Berto
diff mbox

Patch

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. */
@@ -760,7 +763,8 @@  static void bdrv_backing_options(int *child_flags, QDict *child_options,
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
 
     /* backing files always opened read-only */
-    flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ);
+    qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
+    flags &= ~BDRV_O_COPY_ON_READ;
 
     /* snapshot=on is handled on the top layer */
     flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_TEMPORARY);
@@ -807,6 +811,14 @@  static void update_flags_from_options(int *flags, QemuOpts *opts)
     if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
         *flags |= BDRV_O_NOCACHE;
     }
+
+    *flags &= ~BDRV_O_RDWR;
+
+    assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY));
+    if (!qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false)) {
+        *flags |= BDRV_O_RDWR;
+    }
+
 }
 
 static void update_options_from_flags(QDict *options, int flags)
@@ -819,6 +831,10 @@  static void update_options_from_flags(QDict *options, int flags)
         qdict_put(options, BDRV_OPT_CACHE_NO_FLUSH,
                   qbool_from_bool(flags & BDRV_O_NO_FLUSH));
     }
+    if (!qdict_haskey(options, BDRV_OPT_READ_ONLY)) {
+        qdict_put(options, BDRV_OPT_READ_ONLY,
+                  qbool_from_bool(!(flags & BDRV_O_RDWR)));
+    }
 }
 
 static void bdrv_assign_node_name(BlockDriverState *bs,
@@ -882,6 +898,11 @@  static QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Ignore flush requests",
         },
+        {
+            .name = BDRV_OPT_READ_ONLY,
+            .type = QEMU_OPT_BOOL,
+            .help = "Node is opened in read-only mode",
+        },
         { /* end of list */ }
     },
 };
@@ -1626,14 +1647,22 @@  static BlockDriverState *bdrv_open_inherit(const char *filename,
         goto fail;
     }
 
-    if (flags & BDRV_O_RDWR) {
-        flags |= BDRV_O_ALLOW_RDWR;
+    /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
+     * FIXME: we're parsing the QDict to avoid having to create a
+     * QemuOpts just for this, but neither option is optimal. */
+    if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") &&
+        !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) {
+        flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);
+    } else {
+        flags &= ~BDRV_O_RDWR;
     }
 
     if (flags & BDRV_O_SNAPSHOT) {
         snapshot_options = qdict_new();
         bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
                                    flags, options);
+        /* Let bdrv_backing_options() override "read-only" */
+        qdict_del(options, BDRV_OPT_READ_ONLY);
         bdrv_backing_options(&flags, options, flags, options);
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index ba2620f..ded2109 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2971,7 +2971,8 @@  static BlockDriver vvfat_write_target = {
 static void vvfat_qcow_options(int *child_flags, QDict *child_options,
                                int parent_flags, QDict *parent_options)
 {
-    *child_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
+    qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "off");
+    *child_flags = BDRV_O_NO_FLUSH;
 }
 
 static const BdrvChildRole child_vvfat_qcow = {
diff --git a/blockdev.c b/blockdev.c
index 1399fbd..3036af4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -360,9 +360,6 @@  static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     const char *aio;
 
     if (bdrv_flags) {
-        if (!qemu_opt_get_bool(opts, "read-only", false)) {
-            *bdrv_flags |= BDRV_O_RDWR;
-        }
         if (qemu_opt_get_bool(opts, "copy-on-read", false)) {
             *bdrv_flags |= BDRV_O_COPY_ON_READ;
         }
@@ -471,7 +468,7 @@  static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     int bdrv_flags = 0;
     int on_read_error, on_write_error;
     bool account_invalid, account_failed;
-    bool writethrough;
+    bool writethrough, read_only;
     BlockBackend *blk;
     BlockDriverState *bs;
     ThrottleConfig cfg;
@@ -567,6 +564,8 @@  static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         bdrv_flags |= BDRV_O_SNAPSHOT;
     }
 
+    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
+
     /* init */
     if ((!file || !*file) && !qdict_size(bs_opts)) {
         BlockBackendRootState *blk_rs;
@@ -574,7 +573,7 @@  static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         blk = blk_new();
         blk_rs = blk_get_root_state(blk);
         blk_rs->open_flags    = bdrv_flags;
-        blk_rs->read_only     = !(bdrv_flags & BDRV_O_RDWR);
+        blk_rs->read_only     = read_only;
         blk_rs->detect_zeroes = detect_zeroes;
 
         QDECREF(bs_opts);
@@ -588,6 +587,8 @@  static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
          * 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");
         assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
         if (runstate_check(RUN_STATE_INMIGRATE)) {
@@ -657,6 +658,7 @@  static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
     QemuOpts *opts;
     Error *local_error = NULL;
     BlockdevDetectZeroesOptions detect_zeroes;
+    bool read_only;
     int bdrv_flags = 0;
 
     opts = qemu_opts_create(&qemu_root_bds_opts, NULL, 1, errp);
@@ -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");
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         bdrv_flags |= BDRV_O_INACTIVE;
diff --git a/include/block/block.h b/include/block/block.h
index 9f0399f..57ae122 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -107,6 +107,7 @@  typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_WB       "cache.writeback"
 #define BDRV_OPT_CACHE_DIRECT   "cache.direct"
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
+#define BDRV_OPT_READ_ONLY      "read-only"
 
 
 #define BDRV_SECTOR_BITS   9