[Qemu-devel] [PATCH v2 00/10] Misc reopen-related patches

[Qemu-devel] [PATCH v2 00/10] Misc reopen-related patches

[Alberto Garcia]

Hi,

as part of my blockdev-reopen work here's a new set of patches. This
doesn't implement yet the core functionality of the new reopen
command, but it does fix a few things that help us pave the way.
I believe that the next series after this one will be the last.

The main change is the removal of child references from the options
and explicit_options QDicts. This was already discussed in the
previous series[1], and here's the implementation.

Regards,

Berto

[1] https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00474.html

v2:
- Patches 3 and 5: Make comments more explicit. [Max]
- Patch 6: Use qemu_opts_to_qdict() in bdrv_reopen_prepare() to put
  all unprocessed options back into the QDict. [Max]
- Patches 8-10: Use qemu_opt_get_del() and update commit messages to
  reflect the changes in patch 6. [Max]

v1: https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00846.html
- Initial version

Output of backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/10:[----] [--] 'qemu-io: Fix writethrough check in reopen'
002/10:[----] [--] 'file-posix: x-check-cache-dropped should default to false on reopen'
003/10:[0003] [FC] 'block: Remove child references from bs->{options,explicit_options}'
004/10:[----] [--] 'block: Don't look for child references in append_open_options()'
005/10:[0005] [FC] 'block: Allow child references on reopen'
006/10:[down] 'block: Forbid trying to change unsupported options during reopen'
007/10:[----] [--] 'file-posix: Forbid trying to change unsupported options during reopen'
008/10:[0003] [FC] 'block: Allow changing 'discard' on reopen'
009/10:[0002] [FC] 'block: Allow changing 'detect-zeroes' on reopen'
010/10:[0002] [FC] 'block: Allow changing 'force-share' on reopen'

Alberto Garcia (10):
  qemu-io: Fix writethrough check in reopen
  file-posix: x-check-cache-dropped should default to false on reopen
  block: Remove child references from bs->{options,explicit_options}
  block: Don't look for child references in append_open_options()
  block: Allow child references on reopen
  block: Forbid trying to change unsupported options during reopen
  file-posix: Forbid trying to change unsupported options during reopen
  block: Allow changing 'discard' on reopen
  block: Allow changing 'detect-zeroes' on reopen
  block: Allow changing 'force-share' on reopen

 block.c               | 161 +++++++++++++++++++++++++++++++++-----------------
 block/file-posix.c    |   9 ++-
 include/block/block.h |   2 +
 qemu-io-cmds.c        |   2 +-
 4 files changed, 117 insertions(+), 57 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 01/10] qemu-io: Fix writethrough check in reopen

[Alberto Garcia]

"qemu-io reopen" doesn't allow changing the writethrough setting of
the cache, but the check is wrong, causing an error even on a simple
reopen with the default parameters:

   $ qemu-img create -f qcow2 hd.qcow2 1M
   $ qemu-system-x86_64 -monitor stdio -drive if=virtio,file=hd.qcow2
   (qemu) qemu-io virtio0 reopen
   Cannot change cache.writeback: Device attached

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qemu-io-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5bf5f28178..db0b3ee5ef 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2025,7 +2025,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
         return -EINVAL;
     }
 
-    if (writethrough != blk_enable_write_cache(blk) &&
+    if (!writethrough != blk_enable_write_cache(blk) &&
         blk_get_attached_dev(blk))
     {
         error_report("Cannot change cache.writeback: Device attached");
-- 
2.11.0

[Qemu-devel] [PATCH v2 02/10] file-posix: x-check-cache-dropped should default to false on reopen

[Alberto Garcia]

The default value of x-check-cache-dropped is false. There's no reason
to use the previous value as a default in raw_reopen_prepare() because
bdrv_reopen_queue_child() already takes care of putting the old
options in the BDRVReopenState.options QDict.

If x-check-cache-dropped was previously set but is now missing from
the reopen QDict then it should be reset to false.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index fe83cbf0eb..ddac0cc3e6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -851,7 +851,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     }
 
     rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-                                                s->check_cache_dropped);
+                                                false);
 
     if (s->type == FTYPE_CD) {
         rs->open_flags |= O_NONBLOCK;
-- 
2.11.0

[Qemu-devel] [PATCH v2 03/10] block: Remove child references from bs->{options, explicit_options}

[Alberto Garcia]

Block drivers allow opening their children using a reference to an
existing BlockDriverState. These references remain stored in the
'options' and 'explicit_options' QDicts, but we don't need to keep
them once everything is open.

What is more important, these values can become wrong if the children
change:

    $ qemu-img create -f qcow2 hd0.qcow2 10M
    $ qemu-img create -f qcow2 hd1.qcow2 10M
    $ qemu-img create -f qcow2 hd2.qcow2 10M
    $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \
            -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \
            -drive file=hd2.qcow2,node-name=hd2,backing=hd1

After this hd2 has hd1 as its backing file. Now let's remove it using
block_stream:

    (qemu) block_stream hd2 0 hd0.qcow2

Now hd0 is the backing file of hd2, but hd2's options QDicts still
contain backing=hd1.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 0dbb1fcc7b..c764eb731c 100644
--- a/block.c
+++ b/block.c
@@ -2763,12 +2763,15 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         }
     }
 
-    /* Remove all children options from bs->options and bs->explicit_options */
+    /* Remove all children options and references
+     * from bs->options and bs->explicit_options */
     QLIST_FOREACH(child, &bs->children, next) {
         char *child_key_dot;
         child_key_dot = g_strdup_printf("%s.", child->name);
         qdict_extract_subqdict(bs->explicit_options, NULL, child_key_dot);
         qdict_extract_subqdict(bs->options, NULL, child_key_dot);
+        qdict_del(bs->explicit_options, child->name);
+        qdict_del(bs->options, child->name);
         g_free(child_key_dot);
     }
 
@@ -3289,6 +3292,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 {
     BlockDriver *drv;
     BlockDriverState *bs;
+    BdrvChild *child;
     bool old_can_write, new_can_write;
 
     assert(reopen_state != NULL);
@@ -3313,6 +3317,13 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     bs->open_flags         = reopen_state->flags;
     bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 
+    /* Remove child references from bs->options and bs->explicit_options.
+     * Child options were already removed in bdrv_reopen_queue_child() */
+    QLIST_FOREACH(child, &bs->children, next) {
+        qdict_del(bs->explicit_options, child->name);
+        qdict_del(bs->options, child->name);
+    }
+
     bdrv_refresh_limits(bs, NULL);
 
     bdrv_set_perm(reopen_state->bs, reopen_state->perm,
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 03/10] block: Remove child references from bs->{options, explicit_options}

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> Block drivers allow opening their children using a reference to an
> existing BlockDriverState. These references remain stored in the
> 'options' and 'explicit_options' QDicts, but we don't need to keep
> them once everything is open.
> 
> What is more important, these values can become wrong if the children
> change:
> 
>     $ qemu-img create -f qcow2 hd0.qcow2 10M
>     $ qemu-img create -f qcow2 hd1.qcow2 10M
>     $ qemu-img create -f qcow2 hd2.qcow2 10M
>     $ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \
>             -drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \
>             -drive file=hd2.qcow2,node-name=hd2,backing=hd1
> 
> After this hd2 has hd1 as its backing file. Now let's remove it using
> block_stream:
> 
>     (qemu) block_stream hd2 0 hd0.qcow2
> 
> Now hd0 is the backing file of hd2, but hd2's options QDicts still
> contain backing=hd1.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 04/10] block: Don't look for child references in append_open_options()

[Alberto Garcia]

In the previous patch we removed child references from bs->options, so
there's no need to look for them here anymore.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/block.c b/block.c
index c764eb731c..aaa4cf6897 100644
--- a/block.c
+++ b/block.c
@@ -5154,23 +5154,12 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
 {
     const QDictEntry *entry;
     QemuOptDesc *desc;
-    BdrvChild *child;
     bool found_any = false;
 
     for (entry = qdict_first(bs->options); entry;
          entry = qdict_next(bs->options, entry))
     {
-        /* Exclude node-name references to children */
-        QLIST_FOREACH(child, &bs->children, next) {
-            if (!strcmp(entry->key, child->name)) {
-                break;
-            }
-        }
-        if (child) {
-            continue;
-        }
-
-        /* And exclude all non-driver-specific options */
+        /* Exclude all non-driver-specific options */
         for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
             if (!strcmp(qdict_entry_key(entry), desc->name)) {
                 break;
-- 
2.11.0

[Qemu-devel] [PATCH v2 05/10] block: Allow child references on reopen

[Alberto Garcia]

In the previous patches we removed all child references from
bs->{options,explicit_options} because keeping them is useless and
wrong.

Because of this, any attempt to reopen a BlockDriverState using a
child reference as one of its options would result in a failure,
because bdrv_reopen_prepare() would detect that there's a new option
(the child reference) that wasn't present in bs->options.

But passing child references on reopen can be useful. It's a way to
specify a BDS's child without having to pass recursively all of the
child's options, and if the reference points to a different BDS then
this can allow us to replace the child.

However, replacing the child is something that needs to be implemented
case by case and only when it makes sense. For now, this patch allows
passing a child reference as long as it points to the current child of
the BlockDriverState.

It's also important to remember that, as a consequence of the
previous patches, this child reference will be removed from
bs->{options,explicit_options} after the reopening has been completed.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block.c b/block.c
index aaa4cf6897..5223d16f1b 100644
--- a/block.c
+++ b/block.c
@@ -3241,6 +3241,24 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             QObject *new = entry->value;
             QObject *old = qdict_get(reopen_state->bs->options, entry->key);
 
+            /* Allow child references (child_name=node_name) as long as they
+             * point to the current child (i.e. everything stays the same). */
+            if (qobject_type(new) == QTYPE_QSTRING) {
+                BdrvChild *child;
+                QLIST_FOREACH(child, &reopen_state->bs->children, next) {
+                    if (!strcmp(child->name, entry->key)) {
+                        break;
+                    }
+                }
+
+                if (child) {
+                    const char *str = qobject_get_try_str(new);
+                    if (!strcmp(child->bs->node_name, str)) {
+                        continue; /* Found child with this name, skip option */
+                    }
+                }
+            }
+
             /*
              * TODO: When using -drive to specify blockdev options, all values
              * will be strings; however, when using -blockdev, blockdev-add or
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 05/10] block: Allow child references on reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> In the previous patches we removed all child references from
> bs->{options,explicit_options} because keeping them is useless and
> wrong.
> 
> Because of this, any attempt to reopen a BlockDriverState using a
> child reference as one of its options would result in a failure,
> because bdrv_reopen_prepare() would detect that there's a new option
> (the child reference) that wasn't present in bs->options.
> 
> But passing child references on reopen can be useful. It's a way to
> specify a BDS's child without having to pass recursively all of the
> child's options, and if the reference points to a different BDS then
> this can allow us to replace the child.
> 
> However, replacing the child is something that needs to be implemented
> case by case and only when it makes sense. For now, this patch allows
> passing a child reference as long as it points to the current child of
> the BlockDriverState.
> 
> It's also important to remember that, as a consequence of the
> previous patches, this child reference will be removed from
> bs->{options,explicit_options} after the reopening has been completed.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 06/10] block: Forbid trying to change unsupported options during reopen

[Alberto Garcia]

The bdrv_reopen_prepare() function checks all options passed to each
BlockDriverState (in the reopen_state->options QDict) and makes all
necessary preparations to apply the option changes requested by the
user.

Options are removed from the QDict as they are processed, so at the
end of bdrv_reopen_prepare() only the options that can't be changed
are left. Then a loop goes over all remaining options and verifies
that the old and new values are identical, returning an error if
they're not.

The problem is that at the moment there are options that are removed
from the QDict although they can't be changed. The consequence of this
is any modification to any of those options is silently ignored:

   (qemu) qemu-io virtio0 "reopen -o discard=on"

This happens when all options from bdrv_runtime_opts are removed
from the QDict but then only a few of them are processed. Since
it's especially important that "node-name" and "driver" are not
changed, the code puts them back into the QDict so they are checked
at the end of the function. Instead of putting only those two options
back into the QDict, this patch puts all unprocessed options using
qemu_opts_to_qdict().

update_flags_from_options() also needs to be modified to prevent
BDRV_OPT_CACHE_NO_FLUSH, BDRV_OPT_CACHE_DIRECT and BDRV_OPT_READ_ONLY
from going back to the QDict.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 5223d16f1b..1c0f72454a 100644
--- a/block.c
+++ b/block.c
@@ -1094,19 +1094,19 @@ static void update_flags_from_options(int *flags, QemuOpts *opts)
     *flags &= ~BDRV_O_CACHE_MASK;
 
     assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
-    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
+    if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
         *flags |= BDRV_O_NO_FLUSH;
     }
 
     assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
-    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
+    if (qemu_opt_get_bool_del(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)) {
+    if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) {
         *flags |= BDRV_O_RDWR;
     }
 
@@ -3155,7 +3155,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     BlockDriver *drv;
     QemuOpts *opts;
     QDict *orig_reopen_opts;
-    const char *value;
     bool read_only;
 
     assert(reopen_state != NULL);
@@ -3178,17 +3177,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     update_flags_from_options(&reopen_state->flags, opts);
 
-    /* node-name and driver must be unchanged. Put them back into the QDict, so
-     * that they are checked at the end of this function. */
-    value = qemu_opt_get(opts, "node-name");
-    if (value) {
-        qdict_put_str(reopen_state->options, "node-name", value);
-    }
-
-    value = qemu_opt_get(opts, "driver");
-    if (value) {
-        qdict_put_str(reopen_state->options, "driver", value);
-    }
+    /* All other options (including node-name and driver) must be unchanged.
+     * Put them back into the QDict, so that they are checked at the end
+     * of this function. */
+    qemu_opts_to_qdict(opts, reopen_state->options);
 
     /* 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
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 06/10] block: Forbid trying to change unsupported options during reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> The bdrv_reopen_prepare() function checks all options passed to each
> BlockDriverState (in the reopen_state->options QDict) and makes all
> necessary preparations to apply the option changes requested by the
> user.
> 
> Options are removed from the QDict as they are processed, so at the
> end of bdrv_reopen_prepare() only the options that can't be changed
> are left. Then a loop goes over all remaining options and verifies
> that the old and new values are identical, returning an error if
> they're not.
> 
> The problem is that at the moment there are options that are removed
> from the QDict although they can't be changed. The consequence of this
> is any modification to any of those options is silently ignored:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=on"
> 
> This happens when all options from bdrv_runtime_opts are removed
> from the QDict but then only a few of them are processed. Since
> it's especially important that "node-name" and "driver" are not
> changed, the code puts them back into the QDict so they are checked
> at the end of the function. Instead of putting only those two options
> back into the QDict, this patch puts all unprocessed options using
> qemu_opts_to_qdict().
> 
> update_flags_from_options() also needs to be modified to prevent
> BDRV_OPT_CACHE_NO_FLUSH, BDRV_OPT_CACHE_DIRECT and BDRV_OPT_READ_ONLY
> from going back to the QDict.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 07/10] file-posix: Forbid trying to change unsupported options during reopen

[Alberto Garcia]

The file-posix code is used for the "file", "host_device" and
"host_cdrom" drivers, and it allows reopening images. However the only
option that is actually processed is "x-check-cache-dropped", and
changes in all other options (e.g. "filename") are silently ignored:

   (qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file"

While we could allow changing some of the other options, let's keep
things as they are for now but return an error if the user tries to
change any of them.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/file-posix.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ddac0cc3e6..d4ec2cb3dd 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -850,8 +850,13 @@ static int raw_reopen_prepare(BDRVReopenState *state,
         goto out;
     }
 
-    rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-                                                false);
+    rs->check_cache_dropped =
+        qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
+
+    /* This driver's reopen function doesn't currently allow changing
+     * other options, so let's put them back in the original QDict and
+     * bdrv_reopen_prepare() will detect changes and complain. */
+    qemu_opts_to_qdict(opts, state->options);
 
     if (s->type == FTYPE_CD) {
         rs->open_flags |= O_NONBLOCK;
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 07/10] file-posix: Forbid trying to change unsupported options during reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> The file-posix code is used for the "file", "host_device" and
> "host_cdrom" drivers, and it allows reopening images. However the only
> option that is actually processed is "x-check-cache-dropped", and
> changes in all other options (e.g. "filename") are silently ignored:
> 
>    (qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file"
> 
> While we could allow changing some of the other options, let's keep
> things as they are for now but return an error if the user tries to
> change any of them.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/file-posix.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 08/10] block: Allow changing 'discard' on reopen

[Alberto Garcia]

'discard' is one of the basic BlockdevOptions available for all
drivers, but it's not handled by bdrv_reopen_prepare() so any attempt
to change it results in an error:

   (qemu) qemu-io virtio0 "reopen -o discard=on"
   Cannot change the option 'discard'

Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Max Reitz <mreitz@redhat.com>
---
 block.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block.c b/block.c
index 1c0f72454a..3f4b7640fa 100644
--- a/block.c
+++ b/block.c
@@ -3155,6 +3155,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     BlockDriver *drv;
     QemuOpts *opts;
     QDict *orig_reopen_opts;
+    const char *value;
     bool read_only;
 
     assert(reopen_state != NULL);
@@ -3177,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     update_flags_from_options(&reopen_state->flags, opts);
 
+    value = qemu_opt_get_del(opts, "discard");
+    if (value != NULL) {
+        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
+            error_setg(errp, "Invalid discard option");
+            ret = -EINVAL;
+            goto error;
+        }
+    }
+
     /* All other options (including node-name and driver) must be unchanged.
      * Put them back into the QDict, so that they are checked at the end
      * of this function. */
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 08/10] block: Allow changing 'discard' on reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> 'discard' is one of the basic BlockdevOptions available for all
> drivers, but it's not handled by bdrv_reopen_prepare() so any attempt
> to change it results in an error:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=on"
>    Cannot change the option 'discard'
> 
> Since there's no reason why we shouldn't allow changing it and the
> implementation is simple let's just do it.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 08/10] block: Allow changing 'discard' on reopen

[Alberto Garcia]

On Mon 03 Sep 2018 04:34:06 PM CEST, Alberto Garcia <berto@igalia.com> wrote:

> +    value = qemu_opt_get_del(opts, "discard");
> +    if (value != NULL) {
> +        if (bdrv_parse_discard_flags(value, &reopen_state->flags) != 0) {
> +            error_setg(errp, "Invalid discard option");
> +            ret = -EINVAL;
> +            goto error;
> +        }
> +    }

I just realized that we're leaking the string here. qemu_opt_get_del()
expects that the returned value is freed by the caller.

I'll correct all the patches from this series with this problem and
resend the series.

Berto

[Qemu-devel] [PATCH v2 09/10] block: Allow changing 'detect-zeroes' on reopen

[Alberto Garcia]

'detect-zeroes' is one of the basic BlockdevOptions available for all
drivers, but it's not handled by bdrv_reopen_prepare(), so any attempt
to change it results in an error:

   (qemu) qemu-io virtio0 "reopen -o detect-zeroes=on"
   Cannot change the option 'detect-zeroes'

Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Max Reitz <mreitz@redhat.com>
---
 block.c               | 63 +++++++++++++++++++++++++++++++--------------------
 include/block/block.h |  1 +
 2 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/block.c b/block.c
index 3f4b7640fa..3865ef8517 100644
--- a/block.c
+++ b/block.c
@@ -764,6 +764,30 @@ static void bdrv_join_options(BlockDriverState *bs, QDict *options,
     }
 }
 
+static BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes(QemuOpts *opts,
+                                                            int open_flags,
+                                                            Error **errp)
+{
+    Error *local_err = NULL;
+    const char *value = qemu_opt_get_del(opts, "detect-zeroes");
+    BlockdevDetectZeroesOptions detect_zeroes =
+        qapi_enum_parse(&BlockdevDetectZeroesOptions_lookup, value,
+                        BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return detect_zeroes;
+    }
+
+    if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+        !(open_flags & BDRV_O_UNMAP))
+    {
+        error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+                   "without setting discard operation to unmap");
+    }
+
+    return detect_zeroes;
+}
+
 /**
  * Set open flags for a given discard mode
  *
@@ -1328,7 +1352,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     const char *driver_name = NULL;
     const char *node_name = NULL;
     const char *discard;
-    const char *detect_zeroes;
     QemuOpts *opts;
     BlockDriver *drv;
     Error *local_err = NULL;
@@ -1417,29 +1440,12 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
         }
     }
 
-    detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
-    if (detect_zeroes) {
-        BlockdevDetectZeroesOptions value =
-            qapi_enum_parse(&BlockdevDetectZeroesOptions_lookup,
-                            detect_zeroes,
-                            BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
-                            &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            ret = -EINVAL;
-            goto fail_opts;
-        }
-
-        if (value == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-            !(bs->open_flags & BDRV_O_UNMAP))
-        {
-            error_setg(errp, "setting detect-zeroes to unmap is not allowed "
-                             "without setting discard operation to unmap");
-            ret = -EINVAL;
-            goto fail_opts;
-        }
-
-        bs->detect_zeroes = value;
+    bs->detect_zeroes =
+        bdrv_parse_detect_zeroes(opts, bs->open_flags, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail_opts;
     }
 
     if (filename != NULL) {
@@ -3187,6 +3193,14 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         }
     }
 
+    reopen_state->detect_zeroes =
+        bdrv_parse_detect_zeroes(opts, reopen_state->flags, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto error;
+    }
+
     /* All other options (including node-name and driver) must be unchanged.
      * Put them back into the QDict, so that they are checked at the end
      * of this function. */
@@ -3336,6 +3350,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     bs->options            = reopen_state->options;
     bs->open_flags         = reopen_state->flags;
     bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+    bs->detect_zeroes      = reopen_state->detect_zeroes;
 
     /* Remove child references from bs->options and bs->explicit_options.
      * Child options were already removed in bdrv_reopen_queue_child() */
diff --git a/include/block/block.h b/include/block/block.h
index 4e0871aaf9..f71fa5a1c4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -184,6 +184,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
     int flags;
+    BlockdevDetectZeroesOptions detect_zeroes;
     uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 09/10] block: Allow changing 'detect-zeroes' on reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> 'detect-zeroes' is one of the basic BlockdevOptions available for all
> drivers, but it's not handled by bdrv_reopen_prepare(), so any attempt
> to change it results in an error:
> 
>    (qemu) qemu-io virtio0 "reopen -o detect-zeroes=on"
>    Cannot change the option 'detect-zeroes'
> 
> Since there's no reason why we shouldn't allow changing it and the
> implementation is simple let's just do it.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 63 +++++++++++++++++++++++++++++++--------------------
>  include/block/block.h |  1 +
>  2 files changed, 40 insertions(+), 24 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 10/10] block: Allow changing 'force-share' on reopen

[Alberto Garcia]

'force-share' is one of the basic BlockdevOptions available for all
drivers, but it's not handled by bdrv_reopen_prepare() so any attempt
to change it results in a "Cannot change the option" error:

   (qemu) qemu-io virtio0 "reopen -o force-share=on"
   Cannot change the option 'force-share'

Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.

It's worth noting that after this patch the above reopen call will
still return an error -although a different one- if the image is not
read-only:

   (qemu) qemu-io virtio0 "reopen -o force-share=on"
   force-share=on can only be used with read-only images

Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Max Reitz <mreitz@redhat.com>
---
 block.c               | 30 ++++++++++++++++++++++++------
 include/block/block.h |  1 +
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 3865ef8517..7ee29ec9eb 100644
--- a/block.c
+++ b/block.c
@@ -788,6 +788,18 @@ static BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes(QemuOpts *opts,
     return detect_zeroes;
 }
 
+static bool bdrv_parse_force_share(QemuOpts *opts, int flags, Error **errp)
+{
+    bool value = qemu_opt_get_bool_del(opts, BDRV_OPT_FORCE_SHARE, false);
+
+    if (value && (flags & BDRV_O_RDWR)) {
+        error_setg(errp, BDRV_OPT_FORCE_SHARE
+                   "=on can only be used with read-only images");
+    }
+
+    return value;
+}
+
 /**
  * Set open flags for a given discard mode
  *
@@ -1373,12 +1385,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     drv = bdrv_find_format(driver_name);
     assert(drv != NULL);
 
-    bs->force_share = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE, false);
-
-    if (bs->force_share && (bs->open_flags & BDRV_O_RDWR)) {
-        error_setg(errp,
-                   BDRV_OPT_FORCE_SHARE
-                   "=on can only be used with read-only images");
+    bs->force_share = bdrv_parse_force_share(opts, bs->open_flags, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto fail_opts;
     }
@@ -3201,6 +3210,14 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    reopen_state->force_share =
+        bdrv_parse_force_share(opts, reopen_state->flags, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto error;
+    }
+
     /* All other options (including node-name and driver) must be unchanged.
      * Put them back into the QDict, so that they are checked at the end
      * of this function. */
@@ -3351,6 +3368,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     bs->open_flags         = reopen_state->flags;
     bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
     bs->detect_zeroes      = reopen_state->detect_zeroes;
+    bs->force_share        = reopen_state->force_share;
 
     /* Remove child references from bs->options and bs->explicit_options.
      * Child options were already removed in bdrv_reopen_queue_child() */
diff --git a/include/block/block.h b/include/block/block.h
index f71fa5a1c4..a49a027c54 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -185,6 +185,7 @@ typedef struct BDRVReopenState {
     BlockDriverState *bs;
     int flags;
     BlockdevDetectZeroesOptions detect_zeroes;
+    bool force_share;
     uint64_t perm, shared_perm;
     QDict *options;
     QDict *explicit_options;
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 10/10] block: Allow changing 'force-share' on reopen

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

On 2018-09-03 16:34, Alberto Garcia wrote:
> 'force-share' is one of the basic BlockdevOptions available for all
> drivers, but it's not handled by bdrv_reopen_prepare() so any attempt
> to change it results in a "Cannot change the option" error:
> 
>    (qemu) qemu-io virtio0 "reopen -o force-share=on"
>    Cannot change the option 'force-share'
> 
> Since there's no reason why we shouldn't allow changing it and the
> implementation is simple let's just do it.
> 
> It's worth noting that after this patch the above reopen call will
> still return an error -although a different one- if the image is not
> read-only:
> 
>    (qemu) qemu-io virtio0 "reopen -o force-share=on"
>    force-share=on can only be used with read-only images
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               | 30 ++++++++++++++++++++++++------
>  include/block/block.h |  1 +
>  2 files changed, 25 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]