Re: [Qemu-devel] [PATCH 12/34] block: Allow specifying driver-specific options to reopen

[Max Reitz <mreitz@redhat.com>]

On 08.05.2015 19:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c               | 42 +++++++++++++++++++++++++++++++++++++++---
>   block/commit.c        |  4 ++--
>   include/block/block.h |  4 +++-
>   3 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 95dc51e..561cefd 100644
> --- a/block.c
> +++ b/block.c
> @@ -1584,6 +1584,9 @@ typedef struct BlockReopenQueueEntry {
>    *
>    * bs is the BlockDriverState to add to the reopen queue.
>    *
> + * options contains the changed options for the associated bs
> + * (the BlockReopenQueue takes the ownership)
> + *
>    * flags contains the open flags for the associated bs
>    *
>    * returns a pointer to bs_queue, which is either the newly allocated
> @@ -1591,18 +1594,28 @@ typedef struct BlockReopenQueueEntry {
>    *
>    */
>   BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> -                                    BlockDriverState *bs, int flags)
> +                                    BlockDriverState *bs,
> +                                    QDict *options, int flags)
>   {
>       assert(bs != NULL);
>   
>       BlockReopenQueueEntry *bs_entry;
>       BdrvChild *child;
> +    QDict *old_options;
>   
>       if (bs_queue == NULL) {
>           bs_queue = g_new0(BlockReopenQueue, 1);
>           QSIMPLEQ_INIT(bs_queue);
>       }
>   
> +    if (!options) {
> +        options = qdict_new();
> +    }
> +
> +    old_options = qdict_clone_shallow(bs->options);
> +    qdict_join(options, old_options, false);
> +    QDECREF(old_options);
> +
>       /* bdrv_open() masks this flag out */
>       flags &= ~BDRV_O_PROTOCOL;
>   
> @@ -1614,13 +1627,15 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>           }
>   
>           child_flags = child->role->inherit_flags(flags);
> -        bdrv_reopen_queue(bs_queue, child->bs, child_flags);
> +        /* TODO Pass down child flags (backing.*, extents.*, ...) */
> +        bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags);
>       }
>   
>       bs_entry = g_new0(BlockReopenQueueEntry, 1);
>       QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
>   
>       bs_entry->state.bs = bs;
> +    bs_entry->state.options = options;
>       bs_entry->state.flags = flags;
>   
>       return bs_queue;
> @@ -1673,6 +1688,7 @@ cleanup:
>           if (ret && bs_entry->prepared) {
>               bdrv_reopen_abort(&bs_entry->state);
>           }
> +        QDECREF(bs_entry->state.options);
>           g_free(bs_entry);
>       }
>       g_free(bs_queue);
> @@ -1685,7 +1701,7 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
>   {
>       int ret = -1;
>       Error *local_err = NULL;
> -    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
> +    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
>   
>       ret = bdrv_reopen_multiple(queue, &local_err);
>       if (local_err != NULL) {
> @@ -1761,6 +1777,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>           goto error;
>       }
>   
> +    /* Options that are not handled are only okay if they are unchanged
> +     * compared to the old state. It is expected that some options are only
> +     * used for the initial open, but not reopen (e.g. filename) */
> +    if (qdict_size(reopen_state->options)) {
> +        const QDictEntry *entry = qdict_first(reopen_state->options);
> +
> +        do {
> +            QString *new_obj = qobject_to_qstring(entry->value);

Maybe add an assert(new_obj)? If it's NULL, the next qstring_get_str() 
will dereference NULL which is basically just as good, but well...

> +            const char *new = qstring_get_str(new_obj);
> +            const char *old = qdict_get_try_str(reopen_state->bs->options,
> +                                                entry->key);
> +
> +            if (!new != !old || strcmp(new, old)) {

This form implies to me that you expect !new might be true. However, 
assuming that, we might get !new && !old, thus !new == !old and thus 
strcmp(NULL, NULL), which would be bad.

I think !new will never be true. Therefore, I'd rather put this as "if 
(!old || strcmp(new, old))".

> +                error_setg(errp, "Cannot change the option '%s'", entry->key);
> +                ret = -EINVAL;

Hm, well, so far this function generally returned -1 on error, but okay...

I don't see anything truly wrong, though, so with or without assert() 
and the different condition:

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

> +                goto error;
> +            }
> +        } while ((entry = qdict_next(reopen_state->options, entry)));
> +    }
> +
>       ret = 0;
>   
>   error:
> diff --git a/block/commit.c b/block/commit.c
> index cfa2bbe..2c07d12 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -235,11 +235,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>   
>       /* convert base & overlay_bs to r/w, if necessary */
>       if (!(orig_base_flags & BDRV_O_RDWR)) {
> -        reopen_queue = bdrv_reopen_queue(reopen_queue, base,
> +        reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
>                                            orig_base_flags | BDRV_O_RDWR);
>       }
>       if (!(orig_overlay_flags & BDRV_O_RDWR)) {
> -        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
> +        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
>                                            orig_overlay_flags | BDRV_O_RDWR);
>       }
>       if (reopen_queue) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 341a551..1287013 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -146,6 +146,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
>   typedef struct BDRVReopenState {
>       BlockDriverState *bs;
>       int flags;
> +    QDict *options;
>       void *opaque;
>   } BDRVReopenState;
>   
> @@ -215,7 +216,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>                 const char *reference, QDict *options, int flags,
>                 BlockDriver *drv, Error **errp);
>   BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> -                                    BlockDriverState *bs, int flags);
> +                                    BlockDriverState *bs,
> +                                    QDict *options, int flags);
>   int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
>   int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
>   int bdrv_reopen_prepare(BDRVReopenState *reopen_state,