[Qemu-devel] [PATCH 0/9] Misc reopen-related patches

[Qemu-devel] [PATCH 0/9] 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

Alberto Garcia (9):
  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
  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               | 146 +++++++++++++++++++++++++++++++++++---------------
 block/file-posix.c    |   9 +++-
 include/block/block.h |   2 +
 qemu-io-cmds.c        |   2 +-
 4 files changed, 113 insertions(+), 46 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/9] 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>
---
 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 2/9] 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>
---
 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 3/9] 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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 0dbb1fcc7b..2f2484965d 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,12 @@ 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 */
+    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

[Qemu-devel] [PATCH 4/9] 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>
---
 block.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 2f2484965d..92afb3dcce 100644
--- a/block.c
+++ b/block.c
@@ -5153,23 +5153,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 5/9] 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block.c b/block.c
index 92afb3dcce..804d557608 100644
--- a/block.c
+++ b/block.c
@@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             QObject *new = entry->value;
             QObject *old = qdict_get(reopen_state->bs->options, entry->key);
 
+            /* We allow child references 'child_name'='value'
+             * if 'child_name' is an existing child of the BDS
+             * and 'value' is the child's node name (a string). */
+            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

[Qemu-devel] [PATCH 6/9] 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

[Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

[Alberto Garcia]

'discard' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

This should update the discard setting, but does nothing:

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

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>
---
 block.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block.c b/block.c
index 804d557608..21f1eb9cd1 100644
--- a/block.c
+++ b/block.c
@@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     update_flags_from_options(&reopen_state->flags, opts);
 
+    value = qemu_opt_get(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;
+        }
+    }
+
     /* 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");
-- 
2.11.0

[Qemu-devel] [PATCH 8/9] block: Allow changing 'detect-zeroes' on reopen

[Alberto Garcia]

'detect-zeroes' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

This should update the detect-zeroes setting, but does nothing:

   (qemu) qemu-io virtio0 "reopen -o detect-zeroes=on"

This should return an error but also does nothing:

   (qemu) qemu-io virtio0 "reopen -o discard=off,detect-zeroes=unmap"

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>
---
 block.c               | 63 +++++++++++++++++++++++++++++++--------------------
 include/block/block.h |  1 +
 2 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/block.c b/block.c
index 21f1eb9cd1..9aed5c19f4 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(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;
+    }
+
     /* 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");
@@ -3344,6 +3358,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 */
     QLIST_FOREACH(child, &bs->children, next) {
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

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

[Alberto Garcia]

'force-share' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

One example of how this option is being ignored is that on a
read-write image this should produce an error:

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

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>
---
 block.c               | 30 ++++++++++++++++++++++++------
 include/block/block.h |  1 +
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 9aed5c19f4..b6e495b716 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(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;
+    }
+
     /* 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");
@@ -3359,6 +3376,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 */
     QLIST_FOREACH(child, &bs->children, next) {
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 1/9] qemu-io: Fix writethrough check in reopen

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> "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>
> ---
>  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) &&

I'd prefer the ! before the blk_enable_write_cache(), because that's how
the initial assignment of writethrough looks, but nobody really cares, so:

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

>          blk_get_attached_dev(blk))
>      {
>          error_report("Cannot change cache.writeback: Device attached");
> 



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

Re: [Qemu-devel] [PATCH 2/9] file-posix: x-check-cache-dropped should default to false on reopen

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> 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.

Well, the reason would be because one might want to retain unspecified
values as they were set before.  But we decided we don't want to do that.

> 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>
> ---
>  block/file-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

So:

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


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

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

[Max Reitz]

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

On 2018-08-26 16:09, 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 | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

Also, the question is, why would we discard the child options, but keep
the references (which we do not add, so we only have the ones specified
by the user).

> diff --git a/block.c b/block.c
> index 0dbb1fcc7b..2f2484965d 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,12 @@ 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 */
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        qdict_del(bs->explicit_options, child->name);
> +        qdict_del(bs->options, child->name);
> +    }
> +

Why don't you remove the child options as well?

Max

>      bdrv_refresh_limits(bs, NULL);
>  
>      bdrv_set_perm(reopen_state->bs, reopen_state->perm,
> 



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

Re: [Qemu-devel] [PATCH 4/9] block: Don't look for child references in append_open_options()

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> 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>
> ---
>  block.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)

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


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

Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Max Reitz]

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

On 2018-08-26 16:09, 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.

"For now"?  If you intend it to be case-by-case, won't it be handled by
the driver's bdrv_reopen_prepare() implementation?

> 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 | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 92afb3dcce..804d557608 100644
> --- a/block.c
> +++ b/block.c
> @@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>              QObject *new = entry->value;
>              QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>  
> +            /* We allow child references 'child_name'='value'
> +             * if 'child_name' is an existing child of the BDS
> +             * and 'value' is the child's node name (a string). */
> +            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 */
> +                    }
> +                }
> +            }
> +

Looks good, but I think the comment looks a bit convoluted.  Maybe add a
note like "(so the child has to stay the same)"?

Third...  Hm.  This is just a general question.  So as far as I
understand we decided that when you don't specify an option for reopen,
it means that the default is used, and that we don't want to retain the
old value.  Specifically, that means when you don't specify @backing,
the default chain will be opened and may replace the current one.  So
for other children, not specifying them should mean detaching them.

So why isn't it mandatory to re-specify all children for now?

Max


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

Re: [Qemu-devel] [PATCH 6/9] file-posix: Forbid trying to change unsupported options during reopen

[Max Reitz]

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

On 2018-08-26 16:09, 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(-)

Looks OK, but the same question from the last patch arises.  If
unspecified options mean using the default, shouldn't the user have to
re-specify all mandatory options, so at least @filename in this case?

Max


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

Re: [Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> 'discard' is one of the basic BlockdevOptions available for all
> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
> the user cannot change it and doesn't get an error explaining that it
> can't be changed.
> 
> This should update the discard setting, but does nothing:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=on"
> 
> 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>
> ---
>  block.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 804d557608..21f1eb9cd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>  
>      update_flags_from_options(&reopen_state->flags, opts);
>  
> +    value = qemu_opt_get(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;
> +        }
> +    }
> +
>      /* 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");

Hm.  Why not just use the same "trick" here as in your last patch, i.e.
use qemu_opt_get_del() above and then qemu_opts_to_qdict() to return all
unhandled options to the QDict?

(We'd have to use *_del() in update_flags_from_options() as well, but
that would be OK for all callers, I believe.)

Max


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

Re: [Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> 'discard' is one of the basic BlockdevOptions available for all
> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
> the user cannot change it and doesn't get an error explaining that it
> can't be changed.
> 
> This should update the discard setting, but does nothing:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=on"
> 
> 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>
> ---
>  block.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 804d557608..21f1eb9cd1 100644
> --- a/block.c
> +++ b/block.c
> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>  
>      update_flags_from_options(&reopen_state->flags, opts);
>  
> +    value = qemu_opt_get(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;
> +        }
> +    }
> +

Let me add a:

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

Although I think it might need a _del.


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

Re: [Qemu-devel] [PATCH 8/9] block: Allow changing 'detect-zeroes' on reopen

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> 'detect-zeroes' is one of the basic BlockdevOptions available for all
> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
> the user cannot change it and doesn't get an error explaining that it
> can't be changed.
> 
> This should update the detect-zeroes setting, but does nothing:
> 
>    (qemu) qemu-io virtio0 "reopen -o detect-zeroes=on"
> 
> This should return an error but also does nothing:
> 
>    (qemu) qemu-io virtio0 "reopen -o discard=off,detect-zeroes=unmap"
> 
> 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>
> ---
>  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 --]

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

[Alberto Garcia]

On Wed 29 Aug 2018 12:43:06 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, 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 | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> Also, the question is, why would we discard the child options, but
> keep the references (which we do not add, so we only have the ones
> specified by the user).

We discussed this a couple of weeks ago: 

https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00478.html

In the end I got convinced that we didn't need to keep the child
references.

>> @@ -3313,6 +3317,12 @@ 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 */
>> +    QLIST_FOREACH(child, &bs->children, next) {
>> +        qdict_del(bs->explicit_options, child->name);
>> +        qdict_del(bs->options, child->name);
>> +    }
>> +
>
> Why don't you remove the child options as well?

Because there's no child options here at this point. They are removed
from the parent QDict in bdrv_reopen_queue_child():

  QLIST_FOREACH(child, &bs->children, next) {
      /* ... */
      child_key_dot = g_strdup_printf("%s.", child->name);
      qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
      qdict_extract_subqdict(options, &new_child_options, child_key_dot);
      g_free(child_key_dot);

      bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
                              child->role, options, flags);
  }

Berto

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

[Max Reitz]

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

On 2018-08-26 16:09, Alberto Garcia wrote:
> 'force-share' is one of the basic BlockdevOptions available for all
> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
> the user cannot change it and doesn't get an error explaining that it
> can't be changed.
> 
> One example of how this option is being ignored is that on a
> read-write image this should produce an error:
> 
>    (qemu) qemu-io virtio0 "reopen -o force-share=on"
>    force-share=on can only be used with read-only images
> 
> 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>
> ---
>  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 --]

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

[Max Reitz]

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

On 2018-08-29 13:53, Alberto Garcia wrote:
> On Wed 29 Aug 2018 12:43:06 PM CEST, Max Reitz wrote:
>> On 2018-08-26 16:09, 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 | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> Also, the question is, why would we discard the child options, but
>> keep the references (which we do not add, so we only have the ones
>> specified by the user).
> 
> We discussed this a couple of weeks ago: 
> 
> https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00478.html
> 
> In the end I got convinced that we didn't need to keep the child
> references.

OK, so your argument was "because references are indeed options for the
parent".  But as I said, that's only valid if all references are there,
which they aren't necessarily.

(If the user specifies the @file options inline, then you won't get a
reference for @file in bs->options.)

But since you discussed all of that already anyway, it doesn't really
matters.  What matters is that I don't disagree. :-)

>>> @@ -3313,6 +3317,12 @@ 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 */
>>> +    QLIST_FOREACH(child, &bs->children, next) {
>>> +        qdict_del(bs->explicit_options, child->name);
>>> +        qdict_del(bs->options, child->name);
>>> +    }
>>> +
>>
>> Why don't you remove the child options as well?
> 
> Because there's no child options here at this point. They are removed
> from the parent QDict in bdrv_reopen_queue_child():
> 
>   QLIST_FOREACH(child, &bs->children, next) {
>       /* ... */
>       child_key_dot = g_strdup_printf("%s.", child->name);
>       qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
>       qdict_extract_subqdict(options, &new_child_options, child_key_dot);
>       g_free(child_key_dot);
> 
>       bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
>                               child->role, options, flags);
>   }

Hm.  It's a bit weird to split child options and child references into
two parts, but I suppose it makes sense.

We need to handle inline child options before the reopen, because those
are reopen options for the current child (and not intended to replace
anything).  We do not want to remove child references there, because
specifying a reference means attaching a different child.[1]

In commit, we want to remove references, because we don't want them in
bs->*options.  We'd also want to remove inline child options, but as you
say, there is no need to, because they have been removed already.

What do you think of adding a note to make it more clear?
(e.g. "(The inline child options have been handled in
bdrv_reopen_queue_child() already)")

Max


[1] On second thought, we do probably want to check the references
there, at least.  If you attach a new child, there is no need to reopen
the current child, so we should skip the bdrv_reopen_queue_child() -- or
rather, we should call it on the new child the user wants to attach.
But that's not something for this series.


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

Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Alberto Garcia]

On Wed 29 Aug 2018 01:26:46 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, 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.
>
> "For now"?  If you intend it to be case-by-case, won't it be handled by
> the driver's bdrv_reopen_prepare() implementation?

Yes, it will (except the "backing" reference which is handled by the
main bdrv_reopen_prepare() function).

What I meant with "For now" is that this patch doesn't add support for
any other case.

>> @@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>              QObject *new = entry->value;
>>              QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>>  
>> +            /* We allow child references 'child_name'='value'
>> +             * if 'child_name' is an existing child of the BDS
>> +             * and 'value' is the child's node name (a string). */
>> +            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 */
>> +                    }
>> +                }
>> +            }
>> +
>
> Looks good, but I think the comment looks a bit convoluted.  Maybe add
> a note like "(so the child has to stay the same)"?

Ok, I can think of making it shorter.

> Third...  Hm.  This is just a general question.  So as far as I
> understand we decided that when you don't specify an option for
> reopen, it means that the default is used, and that we don't want to
> retain the old value.

Correct.

> Specifically, that means when you don't specify @backing, the default
> chain will be opened and may replace the current one.

At the moment "backing" is the only child option that can be omitted,
all others must be specified. So if you leave "backing" out on reopen()
we have the following possibilities:

  a) We open the default backing file from disk. I don't think we want
     to open a new image on reopen(). Plus, what happens if the current
     backing image is the default one? Do we need to detect that? And
     what if it's the default image but has non-default options? The
     whole thing looks like a can of worms.

  b) We leave the current backing file untouched.

  c) We forbid it, and the user has to pass an explicit child, or NULL.

I chose (b) in this series. I suppose (c) could also be an option. But
(a) doesn't looke like a good idea.

It it inconsistent with the way blockdev-reopen is supposed to work?
Maybe it is, but I think we have to make an exception in this case.

> So why isn't it mandatory to re-specify all children for now?

It is, but "backing" is optional. As I said, we can make it mandatory on
reopen(). Perhaps it's the most consistent choice after all.

Berto

Re: [Qemu-devel] [PATCH 6/9] file-posix: Forbid trying to change unsupported options during reopen

[Alberto Garcia]

On Wed 29 Aug 2018 01:33:13 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, 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(-)
>
> Looks OK, but the same question from the last patch arises.  If
> unspecified options mean using the default, shouldn't the user have to
> re-specify all mandatory options, so at least @filename in this case?

This is the old (existing) reopen command, which takes the current
options and applies the user-specified changes on top. In this one we
can leave options out because the old values are kept.

It's in the new QMP blockdev-reopen command (not in this series yet)
that you need to specify all options (among other things because it
takes a BlockdevOptions struct, so there's no choice not to specify the
mandatory ones).

Berto

Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Max Reitz]

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

On 2018-08-29 14:29, Alberto Garcia wrote:
> On Wed 29 Aug 2018 01:26:46 PM CEST, Max Reitz wrote:
>> On 2018-08-26 16:09, 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.
>>
>> "For now"?  If you intend it to be case-by-case, won't it be handled by
>> the driver's bdrv_reopen_prepare() implementation?
> 
> Yes, it will (except the "backing" reference which is handled by the
> main bdrv_reopen_prepare() function).
> 
> What I meant with "For now" is that this patch doesn't add support for
> any other case.

OK.

>>> @@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>>              QObject *new = entry->value;
>>>              QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>>>  
>>> +            /* We allow child references 'child_name'='value'
>>> +             * if 'child_name' is an existing child of the BDS
>>> +             * and 'value' is the child's node name (a string). */
>>> +            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 */
>>> +                    }
>>> +                }
>>> +            }
>>> +
>>
>> Looks good, but I think the comment looks a bit convoluted.  Maybe add
>> a note like "(so the child has to stay the same)"?
> 
> Ok, I can think of making it shorter.

I didn't really mean "convoluted" in the sense of "it's too much".  It's
mostly just hard to grasp because it tries to be as exact as possible.
Adding a short "colloquial" summary would help.

>> Third...  Hm.  This is just a general question.  So as far as I
>> understand we decided that when you don't specify an option for
>> reopen, it means that the default is used, and that we don't want to
>> retain the old value.
> 
> Correct.
> 
>> Specifically, that means when you don't specify @backing, the default
>> chain will be opened and may replace the current one.
> 
> At the moment "backing" is the only child option that can be omitted,
> all others must be specified. So if you leave "backing" out on reopen()
> we have the following possibilities:
> 
>   a) We open the default backing file from disk. I don't think we want
>      to open a new image on reopen(). Plus, what happens if the current
>      backing image is the default one? Do we need to detect that? And
>      what if it's the default image but has non-default options? The
>      whole thing looks like a can of worms.

True.

>   b) We leave the current backing file untouched.

Hm.  That doesn't make sense for blockdev-add, so you could argue that
this behavior works as a default for reopen (because it cannot be
"confused" with the blockdev-add default).

>   c) We forbid it, and the user has to pass an explicit child, or NULL.

That sounds good.

> I chose (b) in this series. I suppose (c) could also be an option. But
> (a) doesn't looke like a good idea.

I agree with the last sentiment, but I'd prefer making it an error
instead of choosing (b).  Yes, you could argue that choosing (b) as a
default makes sense in a way, but in another, it just doesn't make
sense.  (Because while (b) makes no sense for blockdev-add, (a) would
make sense for both blockdev-add and reopen.  Sure, it's horrible to
support, but from a user's POV, it makes sense.  So it's just confusing
not to make that the default for reopen as well).

But the most important point is that it's trivial for the user to keep
the current backing file.  They just need to give the current backing's
node-name.

So choosing (b) yields no real value over (c), and it may be confusing.
Which is why I prefer (c).

(Whereas (a) would provide real value, because the user has no simple
way of opening the default backing chain.  But I agree that we don't
need it (now), and that going for it would spill the can of worms.)

> It it inconsistent with the way blockdev-reopen is supposed to work?
> Maybe it is, but I think we have to make an exception in this case.

Making it an error would definitely be safe, however.

>> So why isn't it mandatory to re-specify all children for now?
> 
> It is
Is it?  Why does

$ qemu-io -c 'reopen -o lazy-refcounts=on' foo.qcow2

work, then?  Shouldn't I have to specify @file?

Max


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

Re: [Qemu-devel] [PATCH 6/9] file-posix: Forbid trying to change unsupported options during reopen

[Max Reitz]

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

On 2018-08-29 14:33, Alberto Garcia wrote:
> On Wed 29 Aug 2018 01:33:13 PM CEST, Max Reitz wrote:
>> On 2018-08-26 16:09, 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(-)
>>
>> Looks OK, but the same question from the last patch arises.  If
>> unspecified options mean using the default, shouldn't the user have to
>> re-specify all mandatory options, so at least @filename in this case?
> 
> This is the old (existing) reopen command, which takes the current
> options and applies the user-specified changes on top. In this one we
> can leave options out because the old values are kept.
> 
> It's in the new QMP blockdev-reopen command (not in this series yet)
> that you need to specify all options (among other things because it
> takes a BlockdevOptions struct, so there's no choice not to specify the
> mandatory ones).

Hmm...  OK.  I thought we'd just change the current reopen command.  But
that's OK, then.

Max


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

Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Max Reitz]

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

On 2018-08-29 14:59, Max Reitz wrote:
> On 2018-08-29 14:29, Alberto Garcia wrote:
>> On Wed 29 Aug 2018 01:26:46 PM CEST, Max Reitz wrote:

[...]

>>> So why isn't it mandatory to re-specify all children for now?
>>
>> It is
> Is it?  Why does
> 
> $ qemu-io -c 'reopen -o lazy-refcounts=on' foo.qcow2
> 
> work, then?  Shouldn't I have to specify @file?

I suppose the answer is "because that's the old command".  Which is good
enough.

Max


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

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

[Alberto Garcia]

On Wed 29 Aug 2018 02:18:45 PM CEST, Max Reitz wrote:
>>>> @@ -3313,6 +3317,12 @@ 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 */
>>>> +    QLIST_FOREACH(child, &bs->children, next) {
>>>> +        qdict_del(bs->explicit_options, child->name);
>>>> +        qdict_del(bs->options, child->name);
>>>> +    }
>>>> +
>>>
>>> Why don't you remove the child options as well?
>> 
>> Because there's no child options here at this point. They are removed
>> from the parent QDict in bdrv_reopen_queue_child():
>> 
>>   QLIST_FOREACH(child, &bs->children, next) {
>>       /* ... */
>>       child_key_dot = g_strdup_printf("%s.", child->name);
>>       qdict_extract_subqdict(explicit_options, NULL, child_key_dot);
>>       qdict_extract_subqdict(options, &new_child_options, child_key_dot);
>>       g_free(child_key_dot);
>> 
>>       bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
>>                               child->role, options, flags);
>>   }
>
> Hm.  It's a bit weird to split child options and child references into
> two parts, but I suppose it makes sense.

The child references have to remain in the parent until at least
bdrv_reopen_prepare() because we need them if we want to allow replacing
a child.

The child options are removed in bdrv_reopen_queue_child() because we
need to pass them to the children. We could also make a copy and remove
them later in bdrv_reopen_commit() if that makes things more clear, but
I don't know...

> What do you think of adding a note to make it more clear?
> (e.g. "(The inline child options have been handled in
> bdrv_reopen_queue_child() already)")

Sure, why not.

> [1] On second thought, we do probably want to check the references
> there, at least.  If you attach a new child, there is no need to reopen
> the current child, so we should skip the bdrv_reopen_queue_child()

Yes, that's done in a subsequent patch (not part of this series yet).

Berto

Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Alberto Garcia]

On Wed 29 Aug 2018 02:59:01 PM CEST, Max Reitz wrote:
>>> Specifically, that means when you don't specify @backing, the default
>>> chain will be opened and may replace the current one.
>> 
>> At the moment "backing" is the only child option that can be omitted,
>> all others must be specified. So if you leave "backing" out on reopen()
>> we have the following possibilities:
>> 
>>   a) We open the default backing file from disk. I don't think we want
>>      to open a new image on reopen(). Plus, what happens if the current
>>      backing image is the default one? Do we need to detect that? And
>>      what if it's the default image but has non-default options? The
>>      whole thing looks like a can of worms.
>
> True.
>
>>   b) We leave the current backing file untouched.
>
> Hm.  That doesn't make sense for blockdev-add, so you could argue that
> this behavior works as a default for reopen (because it cannot be
> "confused" with the blockdev-add default).
>
>>   c) We forbid it, and the user has to pass an explicit child, or NULL.
>
> That sounds good.
>
>> I chose (b) in this series. I suppose (c) could also be an option. But
>> (a) doesn't looke like a good idea.
>
> I agree with the last sentiment, but I'd prefer making it an error
> instead of choosing (b).  Yes, you could argue that choosing (b) as a
> default makes sense in a way, but in another, it just doesn't make
> sense.  (Because while (b) makes no sense for blockdev-add, (a) would
> make sense for both blockdev-add and reopen.  Sure, it's horrible to
> support, but from a user's POV, it makes sense.  So it's just
> confusing not to make that the default for reopen as well).
>
> But the most important point is that it's trivial for the user to keep
> the current backing file.  They just need to give the current
> backing's node-name.

Or NULL if there's no backing file, right?

The problem I see with this is that it would make the options quite
verbose. "backing" is an attribute of all BDSs, also protocol ones. I
suppose we can make "backing" optional in those, or is there any case
where it makes sense?

Berto

Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Max Reitz]

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

On 2018-08-29 16:56, Alberto Garcia wrote:
> On Wed 29 Aug 2018 02:59:01 PM CEST, Max Reitz wrote:
>>>> Specifically, that means when you don't specify @backing, the default
>>>> chain will be opened and may replace the current one.
>>>
>>> At the moment "backing" is the only child option that can be omitted,
>>> all others must be specified. So if you leave "backing" out on reopen()
>>> we have the following possibilities:
>>>
>>>   a) We open the default backing file from disk. I don't think we want
>>>      to open a new image on reopen(). Plus, what happens if the current
>>>      backing image is the default one? Do we need to detect that? And
>>>      what if it's the default image but has non-default options? The
>>>      whole thing looks like a can of worms.
>>
>> True.
>>
>>>   b) We leave the current backing file untouched.
>>
>> Hm.  That doesn't make sense for blockdev-add, so you could argue that
>> this behavior works as a default for reopen (because it cannot be
>> "confused" with the blockdev-add default).
>>
>>>   c) We forbid it, and the user has to pass an explicit child, or NULL.
>>
>> That sounds good.
>>
>>> I chose (b) in this series. I suppose (c) could also be an option. But
>>> (a) doesn't looke like a good idea.
>>
>> I agree with the last sentiment, but I'd prefer making it an error
>> instead of choosing (b).  Yes, you could argue that choosing (b) as a
>> default makes sense in a way, but in another, it just doesn't make
>> sense.  (Because while (b) makes no sense for blockdev-add, (a) would
>> make sense for both blockdev-add and reopen.  Sure, it's horrible to
>> support, but from a user's POV, it makes sense.  So it's just
>> confusing not to make that the default for reopen as well).
>>
>> But the most important point is that it's trivial for the user to keep
>> the current backing file.  They just need to give the current
>> backing's node-name.
> 
> Or NULL if there's no backing file, right?

Ah, right, that case...  Yes.  Or we could make an exception (if
@backing wasn't specified, and there is no default backing file, then we
can suppress the error and handle it like backing=null).

Right now, I'm not sure whether making an exception is a great idea, but
it's stupid always having to pass backing=null for all qcow2 nodes that
do not have a backing file...

> The problem I see with this is that it would make the options quite
> verbose. "backing" is an attribute of all BDSs, also protocol ones. I
> suppose we can make "backing" optional in those, or is there any case
> where it makes sense?

Right.  Making it optional when there is no default backing file maybe
is less bad than always requiring it.

Max


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

Re: [Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

[Alberto Garcia]

On Wed 29 Aug 2018 01:39:10 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, Alberto Garcia wrote:
>> 'discard' is one of the basic BlockdevOptions available for all
>> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
>> the user cannot change it and doesn't get an error explaining that it
>> can't be changed.
>> 
>> This should update the discard setting, but does nothing:
>> 
>>    (qemu) qemu-io virtio0 "reopen -o discard=on"
>> 
>> 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>
>> ---
>>  block.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index 804d557608..21f1eb9cd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>  
>>      update_flags_from_options(&reopen_state->flags, opts);
>>  
>> +    value = qemu_opt_get(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;
>> +        }
>> +    }
>> +
>>      /* 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");
>
> Hm.  Why not just use the same "trick" here as in your last patch,
> i.e.  use qemu_opt_get_del() above and then qemu_opts_to_qdict() to
> return all unhandled options to the QDict?

Sure, I could do that and therefore forbid changing all of these
options. But as I said in the commit message, there's no reason why we
should not allow changing them.

Berto

Re: [Qemu-devel] [PATCH 7/9] block: Allow changing 'discard' on reopen

[Alberto Garcia]

On Wed 29 Aug 2018 01:39:10 PM CEST, Max Reitz wrote:
> On 2018-08-26 16:09, Alberto Garcia wrote:
>> 'discard' is one of the basic BlockdevOptions available for all
>> drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
>> the user cannot change it and doesn't get an error explaining that it
>> can't be changed.
>> 
>> This should update the discard setting, but does nothing:
>> 
>>    (qemu) qemu-io virtio0 "reopen -o discard=on"
>> 
>> 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>
>> ---
>>  block.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/block.c b/block.c
>> index 804d557608..21f1eb9cd1 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3178,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>>  
>>      update_flags_from_options(&reopen_state->flags, opts);
>>  
>> +    value = qemu_opt_get(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;
>> +        }
>> +    }
>> +
>>      /* 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");
>
> Hm.  Why not just use the same "trick" here as in your last patch,
> i.e.  use qemu_opt_get_del() above and then qemu_opts_to_qdict() to
> return all unhandled options to the QDict?

Hm, I think I misunderstood you earlier. You propose that I use
qemu_opts_to_qdict() to catch all other bdrv_runtime_opts options not
processed in this function.

Yes, that sounds like a good idea.

Berto