Re: [Qemu-devel] [PATCH v4 15/15] block: Assert that flags are up-to-date in bdrv_reopen_prepare()

[Max Reitz <mreitz@redhat.com>]

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

On 07.11.18 13:59, Alberto Garcia wrote:
> Towards the end of bdrv_reopen_queue_child(), before starting to
> process the children, the update_flags_from_options() function is
> called in order to have BDRVReopenState.flags in sync with the options
> from the QDict.
> 
> This is necessary because during the reopen process flags must be
> updated for all nodes in the queue so bdrv_is_writable_after_reopen()
> and the permission checks work correctly.
> 
> Because of that, calling update_flags_from_options() again in
> bdrv_reopen_prepare() doesn't really change the flags (they are
> already up-to-date). But we need to call it in order to remove those
> options from QemuOpts and that way indicate that they have been
> processed.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 68f1e3b45e..03277b3d19 100644
> --- a/block.c
> +++ b/block.c
> @@ -3178,6 +3178,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>                          Error **errp)
>  {
>      int ret = -1;
> +    int old_flags;
>      Error *local_err = NULL;
>      BlockDriver *drv;
>      QemuOpts *opts;
> @@ -3203,7 +3204,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          goto error;
>      }
>  
> +    /* This was already called in bdrv_reopen_queue_child() so the flags
> +     * are up-to-date. This time we simply want to remove the options from
> +     * QemuOpts in order to indicate that they have been processed. */
> +    old_flags = reopen_state->flags;
>      update_flags_from_options(&reopen_state->flags, opts);
> +    assert(old_flags == reopen_state->flags);

Reviewed-by: Max Reitz <mreitz@redhat.com>

Although as my bike-shedding for today I'd like to say that I'd find it
more intuitive to store the "just remove the options" call result into
old_flags instead (or rather something renamed), i.e.

    flags_copy = reopen_state->flags;
    update_flags_from_options(&flags_copy, opts);
    assert(flags_copy == reopen_state->flags);

Not that it matters.

Max


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