[Qemu-devel] [PATCH 0/5] Some bs->options fixes

[Qemu-devel] [PATCH 0/5] Some bs->options fixes

[Alberto Garcia]

Hi everyone,

this is part of the blockdev-reopen work that I'm doing, but since
I'll be away during most of July I thought that I could send already a
couple of patches that I think are ready and don't need anything else
from the rest of the series.

There's two main fixes here:

  1) bs->options are not kept up to date after an image is reopened
     and no longer reflect its state.

  2) bs->options and bs->explicit_options also contain the options of
     a BDS's children, so there's data that is duplicated and will be
     inconsistent as soon as you change the children's options
     directly.

The fix for (2) involves removing all children options from both
QDicts. In the cases of node name references ("backing": "node-name")
those remain in the QDict (they're technically parent options). I
think we don't really need them and it should be possible to get rid
of them, but it's a little more complicated (we need them during
bdrv_reopen() to ensure that the user didn't try to change any of
them).

Regards,

Berto

Alberto Garcia (5):
  qdict: Make qdict_extract_subqdict() accept dst = NULL
  block: Remove children options from bs->{options,explicit_options}
  block: Simplify bdrv_reopen_abort()
  block: Update bs->options if bdrv_reopen() succeeds
  block: Simplify append_open_options()

 block.c               | 42 +++++++++++++++++++++++++++++++-----------
 qobject/block-qdict.c | 11 ++++++++---
 2 files changed, 39 insertions(+), 14 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/5] qdict: Make qdict_extract_subqdict() accept dst = NULL

[Alberto Garcia]

This function extracts all options from a QDict starting with a
certain prefix and puts them in a new QDict.

We'll have a couple of cases where we simply want to discard those
options instead of copying them, and that's what this patch does.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 qobject/block-qdict.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 36129e7379..ef443cb8d5 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -158,20 +158,25 @@ void qdict_flatten(QDict *qdict)
     qdict_flatten_qdict(qdict, qdict, NULL);
 }
 
-/* extract all the src QDict entries starting by start into dst */
+/* extract all the src QDict entries starting by start into dst.
+ * If dst is NULL then the entries are simply removed from src. */
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start)
 
 {
     const QDictEntry *entry, *next;
     const char *p;
 
-    *dst = qdict_new();
+    if (dst) {
+        *dst = qdict_new();
+    }
     entry = qdict_first(src);
 
     while (entry != NULL) {
         next = qdict_next(src, entry);
         if (strstart(entry->key, start, &p)) {
-            qdict_put_obj(*dst, p, qobject_ref(entry->value));
+            if (dst) {
+                qdict_put_obj(*dst, p, qobject_ref(entry->value));
+            }
             qdict_del(src, entry->key);
         }
         entry = next;
-- 
2.11.0

[Qemu-devel] [PATCH 2/5] block: Remove children options from bs->{options, explicit_options}

[Alberto Garcia]

When bdrv_open_inherit() opens a BlockDriverState the options QDict
can contain options for some of its children, passed in the form of
child-name.option=value

So while each child is opened with that subset of options, those same
options remain stored in the parent BDS, leaving (at least) two copies
of each one of them ("child-name.option=value" in the parent and
"option=value" in the child).

Having the children options stored in the parent is unnecessary and it
can easily lead to an inconsistent state:

  $ qemu-img create -f qcow2 hd0.qcow2 10M
  $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
  $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2

  $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1

This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1
using block_stream:

  (qemu) block_stream hd2 0 hd0.qcow2

After this hd2 contains backing.node-name=hd1, which is no longer
correct because hd1 doesn't exist anymore.

This patch removes all children options from the parent dictionaries
at the end of bdrv_open_inherit() and bdrv_reopen_queue_child().

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

diff --git a/block.c b/block.c
index 1b8147c1b3..5aaed424b9 100644
--- a/block.c
+++ b/block.c
@@ -2594,6 +2594,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     BlockBackend *file = NULL;
     BlockDriverState *bs;
     BlockDriver *drv = NULL;
+    BdrvChild *child;
     const char *drvname;
     const char *backing;
     Error *local_err = NULL;
@@ -2777,6 +2778,15 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         }
     }
 
+    /* Remove all children options 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);
+        g_free(child_key_dot);
+    }
+
     bdrv_refresh_filename(bs);
 
     /* Check if any unknown options were used */
@@ -2986,6 +2996,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
         }
 
         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);
 
-- 
2.11.0

[Qemu-devel] [PATCH 3/5] block: Simplify bdrv_reopen_abort()

[Alberto Garcia]

If a bdrv_reopen_multiple() call fails, then the explicit_options
QDict has to be deleted for every entry in the reopen queue. This must
happen regardless of whether that entry's bdrv_reopen_prepare() call
succeeded or not.

This patch simplifies the cleanup code a bit.

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

diff --git a/block.c b/block.c
index 5aaed424b9..48e8f4814c 100644
--- a/block.c
+++ b/block.c
@@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
 
 cleanup:
     QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-        if (ret && bs_entry->prepared) {
-            bdrv_reopen_abort(&bs_entry->state);
-        } else if (ret) {
+        if (ret) {
+            if (bs_entry->prepared) {
+                bdrv_reopen_abort(&bs_entry->state);
+            }
             qobject_unref(bs_entry->state.explicit_options);
         }
         qobject_unref(bs_entry->state.options);
@@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
         drv->bdrv_reopen_abort(reopen_state);
     }
 
-    qobject_unref(reopen_state->explicit_options);
-
     bdrv_abort_perm_update(reopen_state->bs);
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH 4/5] block: Update bs->options if bdrv_reopen() succeeds

[Alberto Garcia]

If bdrv_reopen() succeeds then bs->explicit_options is updated with
the new values, but bs->options never changes.

Here's an example:

   { "execute": "blockdev-add",
     "arguments": {
       "driver": "qcow2",
       "node-name": "hd0",
       "overlap-check": "all",
       "file": {
         "driver": "file",
         "filename": "hd0.qcow2"
       }
     }
   }

After this, both bs->options and bs->explicit_options contain
"overlap-check": "all".

Now let's change that using qemu-io's reopen command:

   (qemu) qemu-io hd0 "reopen -o overlap-check=none"

After this, bs->explicit_options contains the new value but
bs->options still keeps the old one.

This patch updates bs->options after a BDS has been successfully
reopened.

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

diff --git a/block.c b/block.c
index 48e8f4814c..58c8e8e677 100644
--- a/block.c
+++ b/block.c
@@ -3065,8 +3065,8 @@ cleanup:
                 bdrv_reopen_abort(&bs_entry->state);
             }
             qobject_unref(bs_entry->state.explicit_options);
+            qobject_unref(bs_entry->state.options);
         }
-        qobject_unref(bs_entry->state.options);
         g_free(bs_entry);
     }
     g_free(bs_queue);
@@ -3166,6 +3166,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     Error *local_err = NULL;
     BlockDriver *drv;
     QemuOpts *opts;
+    QDict *orig_reopen_opts;
     const char *value;
     bool read_only;
 
@@ -3173,6 +3174,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     assert(reopen_state->bs->drv != NULL);
     drv = reopen_state->bs->drv;
 
+    /* This function and each driver's bdrv_reopen_prepare() remove
+     * entries from reopen_state->options as they are processed, so
+     * we need to make a copy of the original QDict. */
+    orig_reopen_opts = qdict_clone_shallow(reopen_state->options);
+
     /* Process generic block layer options */
     opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
     qemu_opts_absorb_qdict(opts, reopen_state->options, &local_err);
@@ -3279,8 +3285,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
 
     ret = 0;
 
+    /* Restore the original reopen_state->options QDict */
+    qobject_unref(reopen_state->options);
+    reopen_state->options = qobject_ref(orig_reopen_opts);
+
 error:
     qemu_opts_del(opts);
+    qobject_unref(orig_reopen_opts);
     return ret;
 }
 
@@ -3310,8 +3321,10 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 
     /* set BDS specific flags now */
     qobject_unref(bs->explicit_options);
+    qobject_unref(bs->options);
 
     bs->explicit_options   = reopen_state->explicit_options;
+    bs->options            = reopen_state->options;
     bs->open_flags         = reopen_state->flags;
     bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 
-- 
2.11.0

[Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Alberto Garcia]

This function returns a BDS's driver-specific options, excluding also
those from its children. Since we have just removed all children
options from bs->options there's no need to do this last step.

We allow references to children, though ("backing": "node0"), so those
we still have to remove.

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

diff --git a/block.c b/block.c
index 58c8e8e677..9b91b01849 100644
--- a/block.c
+++ b/block.c
@@ -5210,16 +5210,13 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
     QemuOptDesc *desc;
     BdrvChild *child;
     bool found_any = false;
-    const char *p;
 
     for (entry = qdict_first(bs->options); entry;
          entry = qdict_next(bs->options, entry))
     {
-        /* Exclude options for children */
+        /* Exclude node-name references to children */
         QLIST_FOREACH(child, &bs->children, next) {
-            if (strstart(qdict_entry_key(entry), child->name, &p)
-                && (!*p || *p == '.'))
-            {
+            if (!strcmp(entry->key, child->name)) {
                 break;
             }
         }
-- 
2.11.0

Re: [Qemu-devel] [PATCH 0/5] Some bs->options fixes

[Alberto Garcia]

ping

On Fri 29 Jun 2018 01:36:58 PM CEST, Alberto Garcia wrote:
> Hi everyone,
>
> this is part of the blockdev-reopen work that I'm doing, but since
> I'll be away during most of July I thought that I could send already a
> couple of patches that I think are ready and don't need anything else
> from the rest of the series.
>
> There's two main fixes here:
>
>   1) bs->options are not kept up to date after an image is reopened
>      and no longer reflect its state.
>
>   2) bs->options and bs->explicit_options also contain the options of
>      a BDS's children, so there's data that is duplicated and will be
>      inconsistent as soon as you change the children's options
>      directly.
>
> The fix for (2) involves removing all children options from both
> QDicts. In the cases of node name references ("backing": "node-name")
> those remain in the QDict (they're technically parent options). I
> think we don't really need them and it should be possible to get rid
> of them, but it's a little more complicated (we need them during
> bdrv_reopen() to ensure that the user didn't try to change any of
> them).
>
> Regards,
>
> Berto
>
> Alberto Garcia (5):
>   qdict: Make qdict_extract_subqdict() accept dst = NULL
>   block: Remove children options from bs->{options,explicit_options}
>   block: Simplify bdrv_reopen_abort()
>   block: Update bs->options if bdrv_reopen() succeeds
>   block: Simplify append_open_options()
>
>  block.c               | 42 +++++++++++++++++++++++++++++++-----------
>  qobject/block-qdict.c | 11 ++++++++---
>  2 files changed, 39 insertions(+), 14 deletions(-)
>
> -- 
> 2.11.0

Re: [Qemu-devel] [PATCH 3/5] block: Simplify bdrv_reopen_abort()

[Kevin Wolf]

Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
> If a bdrv_reopen_multiple() call fails, then the explicit_options
> QDict has to be deleted for every entry in the reopen queue. This must
> happen regardless of whether that entry's bdrv_reopen_prepare() call
> succeeded or not.
> 
> This patch simplifies the cleanup code a bit.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5aaed424b9..48e8f4814c 100644
> --- a/block.c
> +++ b/block.c
> @@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
>  
>  cleanup:
>      QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> -        if (ret && bs_entry->prepared) {
> -            bdrv_reopen_abort(&bs_entry->state);
> -        } else if (ret) {
> +        if (ret) {
> +            if (bs_entry->prepared) {
> +                bdrv_reopen_abort(&bs_entry->state);
> +            }
>              qobject_unref(bs_entry->state.explicit_options);
>          }
>          qobject_unref(bs_entry->state.options);
> @@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>          drv->bdrv_reopen_abort(reopen_state);
>      }
>  
> -    qobject_unref(reopen_state->explicit_options);
> -
>      bdrv_abort_perm_update(reopen_state->bs);
>  }

I think this is only correct if we make bdrv_reopen_prepare/commit/abort
private. At the moment they are public interfaces, so we must have
thought that some callers might want to integrate them into other
transactions.

Kevin

Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Kevin Wolf]

Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
> This function returns a BDS's driver-specific options, excluding also
> those from its children. Since we have just removed all children
> options from bs->options there's no need to do this last step.
> 
> We allow references to children, though ("backing": "node0"), so those
> we still have to remove.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Hmm, yes, but if I compare this with the example you gave in an earlier
patch:

      $ qemu-img create -f qcow2 hd0.qcow2 10M
      $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
      $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2

      $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1

    This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1
    using block_stream:

      (qemu) block_stream hd2 0 hd0.qcow2

    After this hd2 contains backing.node-name=hd1, which is no longer
    correct because hd1 doesn't exist anymore.

Doesn't backing=hd1 actually result in the same kind of inconsistency?
That is, bs->option will still have backing=hd1, but in reality we
reference hd0 now?

Should we get rid of child references in bs->(explicit_)options as well?

Kevin

Re: [Qemu-devel] [PATCH 3/5] block: Simplify bdrv_reopen_abort()

[Alberto Garcia]

On Tue 14 Aug 2018 11:07:42 AM CEST, Kevin Wolf wrote:
> Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
>> If a bdrv_reopen_multiple() call fails, then the explicit_options
>> QDict has to be deleted for every entry in the reopen queue. This must
>> happen regardless of whether that entry's bdrv_reopen_prepare() call
>> succeeded or not.
>> 
>> This patch simplifies the cleanup code a bit.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 5aaed424b9..48e8f4814c 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
>>  
>>  cleanup:
>>      QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
>> -        if (ret && bs_entry->prepared) {
>> -            bdrv_reopen_abort(&bs_entry->state);
>> -        } else if (ret) {
>> +        if (ret) {
>> +            if (bs_entry->prepared) {
>> +                bdrv_reopen_abort(&bs_entry->state);
>> +            }
>>              qobject_unref(bs_entry->state.explicit_options);
>>          }
>>          qobject_unref(bs_entry->state.options);
>> @@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>>          drv->bdrv_reopen_abort(reopen_state);
>>      }
>>  
>> -    qobject_unref(reopen_state->explicit_options);
>> -
>>      bdrv_abort_perm_update(reopen_state->bs);
>>  }
>
> I think this is only correct if we make bdrv_reopen_prepare/commit/abort
> private. At the moment they are public interfaces, so we must have
> thought that some callers might want to integrate them into other
> transactions.

But that's not a problem as long as the new callers unref
state.explicit_options when bdrv_reopen_prepare() fails. They need to do
it with state.options anyway.

Berto

Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Alberto Garcia]

On Tue 14 Aug 2018 11:17:50 AM CEST, Kevin Wolf wrote:
> Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
>> This function returns a BDS's driver-specific options, excluding also
>> those from its children. Since we have just removed all children
>> options from bs->options there's no need to do this last step.
>> 
>> We allow references to children, though ("backing": "node0"), so those
>> we still have to remove.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> Hmm, yes, but if I compare this with the example you gave in an earlier
> patch:
>
>       $ qemu-img create -f qcow2 hd0.qcow2 10M
>       $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
>       $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2
>
>       $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1
>
>     This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1
>     using block_stream:
>
>       (qemu) block_stream hd2 0 hd0.qcow2
>
>     After this hd2 contains backing.node-name=hd1, which is no longer
>     correct because hd1 doesn't exist anymore.
>
> Doesn't backing=hd1 actually result in the same kind of inconsistency?
> That is, bs->option will still have backing=hd1, but in reality we
> reference hd0 now?
>
> Should we get rid of child references in bs->(explicit_)options as
> well?

I don't think so, at least not in general.

The main difference is that if you set backing.node-name=foo then it
means that 'node-name=foo' is an option of the child, the option doesn't
belong in the parent at all. So once it's transferred to the child
there's no reason why it shoud remain in the parent. It's redundant and
can interfere with the reopen operation (e.g. you change the option in
the child but when you reopen the parent it will try to revert the child
option to the previous value).

In the case of 'backing=foo' that's clearly an option of the parent,
there's no other place to put it. We could probably remove it as well,
but we have to see carefully that no parent needs to keep that
information. I think we want to keep child references because in general
we don't allow them to be changed in reopen.

Example: you cannot pass 'file=bar' on reopen unless 'bar' was already
the existing value of 'file' (i.e. you're not changing anything). We
need to look for the previous value in bs->options in order to know
that.

After x-blockdev-reopen is ready, 'backing' will perhaps be the
first/only exception, because we'll be able to change it and the
previous value doesn't matter.

But that's part of the patches I'm working on.

Berto

Re: [Qemu-devel] [PATCH 3/5] block: Simplify bdrv_reopen_abort()

[Kevin Wolf]

Am 14.08.2018 um 12:15 hat Alberto Garcia geschrieben:
> On Tue 14 Aug 2018 11:07:42 AM CEST, Kevin Wolf wrote:
> > Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
> >> If a bdrv_reopen_multiple() call fails, then the explicit_options
> >> QDict has to be deleted for every entry in the reopen queue. This must
> >> happen regardless of whether that entry's bdrv_reopen_prepare() call
> >> succeeded or not.
> >> 
> >> This patch simplifies the cleanup code a bit.
> >> 
> >> Signed-off-by: Alberto Garcia <berto@igalia.com>
> >> ---
> >>  block.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/block.c b/block.c
> >> index 5aaed424b9..48e8f4814c 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
> >>  
> >>  cleanup:
> >>      QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> >> -        if (ret && bs_entry->prepared) {
> >> -            bdrv_reopen_abort(&bs_entry->state);
> >> -        } else if (ret) {
> >> +        if (ret) {
> >> +            if (bs_entry->prepared) {
> >> +                bdrv_reopen_abort(&bs_entry->state);
> >> +            }
> >>              qobject_unref(bs_entry->state.explicit_options);
> >>          }
> >>          qobject_unref(bs_entry->state.options);
> >> @@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> >>          drv->bdrv_reopen_abort(reopen_state);
> >>      }
> >>  
> >> -    qobject_unref(reopen_state->explicit_options);
> >> -
> >>      bdrv_abort_perm_update(reopen_state->bs);
> >>  }
> >
> > I think this is only correct if we make bdrv_reopen_prepare/commit/abort
> > private. At the moment they are public interfaces, so we must have
> > thought that some callers might want to integrate them into other
> > transactions.
> 
> But that's not a problem as long as the new callers unref
> state.explicit_options when bdrv_reopen_prepare() fails. They need to do
> it with state.options anyway.

Actually that makes sense because the code creating the object isn't
even in bdrv_reopen_prepare(), but in bdrv_reopen_queue_child(). So it's
the caller that owns the object and should free them too. I just
confused the two functions.

Kevin

Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Kevin Wolf]

Am 14.08.2018 um 12:52 hat Alberto Garcia geschrieben:
> On Tue 14 Aug 2018 11:17:50 AM CEST, Kevin Wolf wrote:
> > Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:
> >> This function returns a BDS's driver-specific options, excluding also
> >> those from its children. Since we have just removed all children
> >> options from bs->options there's no need to do this last step.
> >> 
> >> We allow references to children, though ("backing": "node0"), so those
> >> we still have to remove.
> >> 
> >> Signed-off-by: Alberto Garcia <berto@igalia.com>
> >
> > Hmm, yes, but if I compare this with the example you gave in an earlier
> > patch:
> >
> >       $ qemu-img create -f qcow2 hd0.qcow2 10M
> >       $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
> >       $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2
> >
> >       $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1
> >
> >     This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1
> >     using block_stream:
> >
> >       (qemu) block_stream hd2 0 hd0.qcow2
> >
> >     After this hd2 contains backing.node-name=hd1, which is no longer
> >     correct because hd1 doesn't exist anymore.
> >
> > Doesn't backing=hd1 actually result in the same kind of inconsistency?
> > That is, bs->option will still have backing=hd1, but in reality we
> > reference hd0 now?
> >
> > Should we get rid of child references in bs->(explicit_)options as
> > well?
> 
> I don't think so, at least not in general.
> 
> The main difference is that if you set backing.node-name=foo then it
> means that 'node-name=foo' is an option of the child, the option doesn't
> belong in the parent at all. So once it's transferred to the child
> there's no reason why it shoud remain in the parent. It's redundant and
> can interfere with the reopen operation (e.g. you change the option in
> the child but when you reopen the parent it will try to revert the child
> option to the previous value).

That's a rather reopen-centric point of view, though.

Redundant information isn't nice already, but what really makes it a
problem is that it's potentially incorrect information because it isn't
kept up to date. There is no point in keeping incorrect information, so
I agree that deleting it is best.

> In the case of 'backing=foo' that's clearly an option of the parent,
> there's no other place to put it. We could probably remove it as well,
> but we have to see carefully that no parent needs to keep that
> information. I think we want to keep child references because in general
> we don't allow them to be changed in reopen.
> 
> Example: you cannot pass 'file=bar' on reopen unless 'bar' was already
> the existing value of 'file' (i.e. you're not changing anything). We
> need to look for the previous value in bs->options in order to know
> that.

My point is the backing=foo has the same problem: Storing it in
bs->options is not only redundant, but potentially incorrect because we
never update it when we modify the graph. There is no point in keeping
potentially incorrect information.

When you actually use that incorrect information, you've got a bug.
Reopen with file=bar doesn't have to check whether 'file=bar' is in
bs->options (because that may be outdated information), but whether the
BdrvChild with the name 'file' points to a node called 'bar'.

Getting rid of the potentially incorrect information will make it more
obvious what you have to check to make things work correctly.

> After x-blockdev-reopen is ready, 'backing' will perhaps be the
> first/only exception, because we'll be able to change it and the
> previous value doesn't matter.

I certainly hope that it will not be the only kind of node references
that you can change. Adding/removing filter nodes requires to be able to
change more or less any kind of node references. But we'll see.

Kevin

Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Alberto Garcia]

On Tue 14 Aug 2018 01:14:42 PM CEST, Kevin Wolf wrote:
>> The main difference is that if you set backing.node-name=foo then it
>> means that 'node-name=foo' is an option of the child, the option
>> doesn't belong in the parent at all. So once it's transferred to the
>> child there's no reason why it shoud remain in the parent. It's
>> redundant and can interfere with the reopen operation (e.g. you
>> change the option in the child but when you reopen the parent it will
>> try to revert the child option to the previous value).
>
> That's a rather reopen-centric point of view, though.
>
> Redundant information isn't nice already, but what really makes it a
> problem is that it's potentially incorrect information because it
> isn't kept up to date. There is no point in keeping incorrect
> information, so I agree that deleting it is best.

Yes, it's indeed reopen-centric :-) I agree with you though, the reopen
problem is a practical example of the consequences of keeping both
options, but the main problem is that they can be different and
therefore wrong.

>> In the case of 'backing=foo' that's clearly an option of the parent,
>> there's no other place to put it. We could probably remove it as
>> well, but we have to see carefully that no parent needs to keep that
>> information. I think we want to keep child references because in
>> general we don't allow them to be changed in reopen.
>> 
>> Example: you cannot pass 'file=bar' on reopen unless 'bar' was
>> already the existing value of 'file' (i.e. you're not changing
>> anything). We need to look for the previous value in bs->options in
>> order to know that.
>
> My point is the backing=foo has the same problem: Storing it in
> bs->options is not only redundant, but potentially incorrect because
> we never update it when we modify the graph. There is no point in
> keeping potentially incorrect information.

I tend to agree, but there's one reason why it's not exactly the same
case: with "backing=foo" we know not only that the backing node name is
'foo', but that it was added using a reference rather than with
backing.node-name=foo. I'm not sure if that's information that we need
for anything, however (probably not).

> When you actually use that incorrect information, you've got a bug.
> Reopen with file=bar doesn't have to check whether 'file=bar' is in
> bs->options (because that may be outdated information), but whether
> the BdrvChild with the name 'file' points to a node called 'bar'.

Again, this is correct if we open the BDS with

  file.node-name=bar

and then we allow reopening it with

  file=bar

Different set of options, but the result still makes sense. For this we
need a specific check to verify that this is correct. For the current
behavior we don't need anything now: we only allow the exact same
option.

>> After x-blockdev-reopen is ready, 'backing' will perhaps be the
>> first/only exception, because we'll be able to change it and the
>> previous value doesn't matter.
>
> I certainly hope that it will not be the only kind of node references
> that you can change. Adding/removing filter nodes requires to be able
> to change more or less any kind of node references. But we'll see.

No, but anything that we allow changing has to be done on a case-by-case
basis. You can't simply allow changing any child reference, the specific
driver has to be modified in order to allow that.

Until the driver is changed, the current behavior prevails: if an option
was modified, we return an error.

Berto

Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Kevin Wolf]

Am 14.08.2018 um 13:48 hat Alberto Garcia geschrieben:
> >> In the case of 'backing=foo' that's clearly an option of the parent,
> >> there's no other place to put it. We could probably remove it as
> >> well, but we have to see carefully that no parent needs to keep that
> >> information. I think we want to keep child references because in
> >> general we don't allow them to be changed in reopen.
> >> 
> >> Example: you cannot pass 'file=bar' on reopen unless 'bar' was
> >> already the existing value of 'file' (i.e. you're not changing
> >> anything). We need to look for the previous value in bs->options in
> >> order to know that.
> >
> > My point is the backing=foo has the same problem: Storing it in
> > bs->options is not only redundant, but potentially incorrect because
> > we never update it when we modify the graph. There is no point in
> > keeping potentially incorrect information.
> 
> I tend to agree, but there's one reason why it's not exactly the same
> case: with "backing=foo" we know not only that the backing node name is
> 'foo', but that it was added using a reference rather than with
> backing.node-name=foo. I'm not sure if that's information that we need
> for anything, however (probably not).

Isn't the proper way to check this foo->inherits_from?

But generally, it shouldn't make a difference for most purposes which
way a node was created.

> > When you actually use that incorrect information, you've got a bug.
> > Reopen with file=bar doesn't have to check whether 'file=bar' is in
> > bs->options (because that may be outdated information), but whether
> > the BdrvChild with the name 'file' points to a node called 'bar'.
> 
> Again, this is correct if we open the BDS with
> 
>   file.node-name=bar
> 
> and then we allow reopening it with
> 
>   file=bar
> 
> Different set of options, but the result still makes sense. For this we
> need a specific check to verify that this is correct. For the current
> behavior we don't need anything now: we only allow the exact same
> option.

That's yet another case and another reason why we want to check
BdrvChild instead. If we take BlockdevOptions for blockdev-reopen, you
need to put something there for nodes that you created inline
originally, and just putting the node name there probably makes the most
sense.

Anyway, the case that I had in mind is the case where you removed a
backing file with a block job:

    base <- mid <- top

You stream top into mid, so at the end of the job, mid goes away and
base is the backing file of top. But since you opened top with
backing=mid, that's still what you get when you look at bs->options.

    base <- top
            (backing=mid)

If you now reopen top, and bdrv_reopen looks at bs->options to check
whether the operation is valid, you must specify backing=mid instead of
the correct backing=base so that reopen thinks it's unchanged.

> >> After x-blockdev-reopen is ready, 'backing' will perhaps be the
> >> first/only exception, because we'll be able to change it and the
> >> previous value doesn't matter.
> >
> > I certainly hope that it will not be the only kind of node references
> > that you can change. Adding/removing filter nodes requires to be able
> > to change more or less any kind of node references. But we'll see.
> 
> No, but anything that we allow changing has to be done on a case-by-case
> basis. You can't simply allow changing any child reference, the specific
> driver has to be modified in order to allow that.
> 
> Until the driver is changed, the current behavior prevails: if an option
> was modified, we return an error.

Makes sense to err on the safe side, though I expect that most drivers
don't need to do much more than just allowing the switch.

Maybe, if we want to keep things a bit safer, what we can do is check
that the same node is addressed when you skip all filters.

Kevin

Re: [Qemu-devel] [PATCH 5/5] block: Simplify append_open_options()

[Alberto Garcia]

On Tue 14 Aug 2018 02:08:26 PM CEST, Kevin Wolf wrote:
>> > When you actually use that incorrect information, you've got a bug.
>> > Reopen with file=bar doesn't have to check whether 'file=bar' is in
>> > bs->options (because that may be outdated information), but whether
>> > the BdrvChild with the name 'file' points to a node called 'bar'.
>> 
>> Again, this is correct if we open the BDS with
>> 
>>   file.node-name=bar
>> 
>> and then we allow reopening it with
>> 
>>   file=bar
>> 
>> Different set of options, but the result still makes sense. For this
>> we need a specific check to verify that this is correct. For the
>> current behavior we don't need anything now: we only allow the exact
>> same option.
>
> That's yet another case and another reason why we want to check
> BdrvChild instead. If we take BlockdevOptions for blockdev-reopen, you
> need to put something there for nodes that you created inline
> originally, and just putting the node name there probably makes the
> most sense.
>
> Anyway, the case that I had in mind is the case where you removed a
> backing file with a block job:
>
>     base <- mid <- top
>
> You stream top into mid, so at the end of the job, mid goes away and
> base is the backing file of top. But since you opened top with
> backing=mid, that's still what you get when you look at bs->options.
>
>     base <- top
>             (backing=mid)
>
> If you now reopen top, and bdrv_reopen looks at bs->options to check
> whether the operation is valid, you must specify backing=mid instead
> of the correct backing=base so that reopen thinks it's unchanged.

Yes, that's correct, I'm aware of that problem.

>> >> After x-blockdev-reopen is ready, 'backing' will perhaps be the
>> >> first/only exception, because we'll be able to change it and the
>> >> previous value doesn't matter.
>> >
>> > I certainly hope that it will not be the only kind of node references
>> > that you can change. Adding/removing filter nodes requires to be able
>> > to change more or less any kind of node references. But we'll see.
>> 
>> No, but anything that we allow changing has to be done on a case-by-case
>> basis. You can't simply allow changing any child reference, the specific
>> driver has to be modified in order to allow that.
>> 
>> Until the driver is changed, the current behavior prevails: if an option
>> was modified, we return an error.
>
> Makes sense to err on the safe side, though I expect that most drivers
> don't need to do much more than just allowing the switch.
>
> Maybe, if we want to keep things a bit safer, what we can do is check
> that the same node is addressed when you skip all filters.

The proper fix could be something alone those lines, yes. I'll give it a
try and see what happens.

This would be a separate patch, though, the ones that I sent shouldn't
need changes.

Berto

Re: [Qemu-devel] [PATCH 0/5] Some bs->options fixes

[Kevin Wolf]

Am 29.06.2018 um 13:36 hat Alberto Garcia geschrieben:
> Hi everyone,
> 
> this is part of the blockdev-reopen work that I'm doing, but since
> I'll be away during most of July I thought that I could send already a
> couple of patches that I think are ready and don't need anything else
> from the rest of the series.
> 
> There's two main fixes here:
> 
>   1) bs->options are not kept up to date after an image is reopened
>      and no longer reflect its state.
> 
>   2) bs->options and bs->explicit_options also contain the options of
>      a BDS's children, so there's data that is duplicated and will be
>      inconsistent as soon as you change the children's options
>      directly.
> 
> The fix for (2) involves removing all children options from both
> QDicts. In the cases of node name references ("backing": "node-name")
> those remain in the QDict (they're technically parent options). I
> think we don't really need them and it should be possible to get rid
> of them, but it's a little more complicated (we need them during
> bdrv_reopen() to ensure that the user didn't try to change any of
> them).

Thanks, applied to the block branch.

Kevin