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
> ---
> 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(, 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(_runtime_opts, NULL, 0, _abort);
> qemu_opts_absorb_qdict(opts, options_copy, _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);
> -
