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 <be...@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);
> }
>
> /*
>
signature.asc
Description: OpenPGP digital signature
