[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 3/9] block: Remove child references from bs->{op

From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH 3/9] block: Remove child references from bs->{options, explicit_options}
Date: Wed, 29 Aug 2018 16:52:18 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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).


reply via email to

[Prev in Thread] Current Thread [Next in Thread]