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