Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Max Reitz]

On 2018-08-26 16:09, Alberto Garcia wrote:
> In the previous patches we removed all child references from
> bs->{options,explicit_options} because keeping them is useless and
> wrong.
> 
> Because of this, any attempt to reopen a BlockDriverState using a
> child reference as one of its options would result in a failure,
> because bdrv_reopen_prepare() would detect that there's a new option
> (the child reference) that wasn't present in bs->options.
> 
> But passing child references on reopen can be useful. It's a way to
> specify a BDS's child without having to pass recursively all of the
> child's options, and if the reference points to a different BDS then
> this can allow us to replace the child.
> 
> However, replacing the child is something that needs to be implemented
> case by case and only when it makes sense. For now, this patch allows
> passing a child reference as long as it points to the current child of
> the BlockDriverState.

"For now"?  If you intend it to be case-by-case, won't it be handled by
the driver's bdrv_reopen_prepare() implementation?

> It's also important to remember that, as a consequence of the
> previous patches, this child reference will be removed from
> bs->{options,explicit_options} after the reopening has been completed.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 92afb3dcce..804d557608 100644
> --- a/block.c
> +++ b/block.c
> @@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>              QObject *new = entry->value;
>              QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>  
> +            /* We allow child references 'child_name'='value'
> +             * if 'child_name' is an existing child of the BDS
> +             * and 'value' is the child's node name (a string). */
> +            if (qobject_type(new) == QTYPE_QSTRING) {
> +                BdrvChild *child;
> +                QLIST_FOREACH(child, &reopen_state->bs->children, next) {
> +                    if (!strcmp(child->name, entry->key)) {
> +                        break;
> +                    }
> +                }
> +
> +                if (child) {
> +                    const char *str = qobject_get_try_str(new);
> +                    if (!strcmp(child->bs->node_name, str)) {
> +                        continue; /* Found child with this name, skip option 
> */
> +                    }
> +                }
> +            }
> +

Looks good, but I think the comment looks a bit convoluted.  Maybe add a
note like "(so the child has to stay the same)"?

Third...  Hm.  This is just a general question.  So as far as I
understand we decided that when you don't specify an option for reopen,
it means that the default is used, and that we don't want to retain the
old value.  Specifically, that means when you don't specify @backing,
the default chain will be opened and may replace the current one.  So
for other children, not specifying them should mean detaching them.

So why isn't it mandatory to re-specify all children for now?

Max

signature.asc
Description: OpenPGP digital signature