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
