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

[Max Reitz]

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 <address@hidden>
>>> ---
>>>  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.

signature.asc
Description: OpenPGP digital signature