Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6302 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> Now that all callers are passing the new options using the QDict we no
> longer need the 'flags' parameter.
> 
> This patch makes the following changes:
> 
>    1) The update_options_from_flags() call is no longer necessary
>       so it can be removed.
> 
>    2) The update_flags_from_options() call is now used in all cases,
>       and is moved down a few lines so it happens after the options
>       QDict contains the final set of values.
> 
>    3) The flags parameter is removed. Now the flags are initialized
>       using the current value (for the top-level node) or the parent
>       flags (after inherit_options()). In both cases the initial
>       values are updated to reflect the new options in the QDict. This
>       happens in bdrv_reopen_queue_child() (as explained above) and in
>       bdrv_reopen_prepare().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 50 +++++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fd27b204d9..26c6a4e58a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2884,7 +2884,6 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
>  static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>                                                   BlockDriverState *bs,
>                                                   QDict *options,
> -                                                 int flags,
>                                                   const BdrvChildRole *role,
>                                                   QDict *parent_options,
>                                                   int parent_flags)
> @@ -2894,6 +2893,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      BlockReopenQueueEntry *bs_entry;
>      BdrvChild *child;
>      QDict *old_options, *explicit_options;
> +    int flags;
>  
>      /* Make sure that the caller remembered to use a drained section. This is
>       * important to avoid graph changes between the recursive queuing here and
> @@ -2919,22 +2919,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      /*
>       * Precedence of options:
>       * 1. Explicitly passed in options (highest)
> -     * 2. Set in flags (only for top level)
> -     * 3. Retained from explicitly set options of bs
> -     * 4. Inherited from parent node
> -     * 5. Retained from effective options of bs
> +     * 2. Retained from explicitly set options of bs
> +     * 3. Inherited from parent node
> +     * 4. Retained from effective options of bs
>       */
>  
> -    if (!parent_options) {
> -        /*
> -         * Any setting represented by flags is always updated. If the
> -         * corresponding QDict option is set, it takes precedence. Otherwise
> -         * the flag is translated into a QDict option. The old setting of bs is
> -         * not considered.
> -         */
> -        update_options_from_flags(options, flags);
> -    }
> -
>      /* Old explicitly set values (don't overwrite by inherited value) */
>      if (bs_entry) {
>          old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
> @@ -2948,12 +2937,22 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>  
>      /* Inherit from parent node */
>      if (parent_options) {
> -        QemuOpts *opts;
> -        QDict *options_copy;
> -        Error *local_err = NULL;
> -        assert(!flags);
> +        flags = 0;
>          role->inherit_options(&flags, options, parent_flags, parent_options);
> -        options_copy = qdict_clone_shallow(options);
> +    } else {
> +        flags = bdrv_get_flags(bs);
> +    }
> +
> +    /* Old values are used for options that aren't set yet */
> +    old_options = qdict_clone_shallow(bs->options);
> +    bdrv_join_options(bs, options, old_options);
> +    qobject_unref(old_options);
> +
> +    /* We have the final set of options so let's update the flags */
> +    {
> +        Error *local_err = NULL;
> +        QemuOpts *opts;
> +        QDict *options_copy = qdict_clone_shallow(options);

I-I'm not sure this conforms to our coding style.

While I applaud your effort to keep the patch size small, I know for
sure I don't want to read code like this.  (Unless it's necessary
because of some variables' lifetimes.)

I agree with the changes, however.

Max

>          opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
>          qemu_opts_absorb_qdict(opts, options_copy, &local_err);
>          /* Don't call update_flags_from_options() if the options are wrong.
> @@ -2967,11 +2966,6 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>          qobject_unref(options_copy);
>      }
>  
> -    /* Old values are used for options that aren't set yet */
> -    old_options = qdict_clone_shallow(bs->options);
> -    bdrv_join_options(bs, options, old_options);
> -    qobject_unref(old_options);
> -
>      /* bdrv_open_inherit() sets and clears some additional flags internally */
>      flags &= ~BDRV_O_PROTOCOL;
>      if (flags & BDRV_O_RDWR) {
> @@ -3011,7 +3005,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>          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,
> +        bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
>                                  child->role, options, flags);
>      }
>  
> @@ -3022,9 +3016,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                      BlockDriverState *bs,
>                                      QDict *options)
>  {
> -    int flags = bdrv_get_flags(bs);
> -    return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
> -                                   NULL, NULL, 0);
> +    return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, NULL, 0);
>  }
>  
>  /*
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]