Re: [Qemu-devel] [PATCH 2/4] block: introduce bdrv_runtime_opts

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> This patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList.
> The list will absorb all options that belong to the BDS (and not the
> BlockBackend) and will be parsed and handled in bdrv_open_common.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Signed-off-by: Peter Lieven <address@hidden>
> ---
>   block.c |   39 +++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index ba5281d..3b4a59a 100644
> --- a/block.c
> +++ b/block.c
> @@ -27,6 +27,7 @@
>   #include "block/block_int.h"
>   #include "block/blockjob.h"
>   #include "qemu/module.h"
> +#include "qapi/qmp/qbool.h"
>   #include "qapi/qmp/qjson.h"
>   #include "sysemu/block-backend.h"
>   #include "sysemu/sysemu.h"
> @@ -875,6 +876,19 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
>       QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
>   }
>   
> +static QemuOptsList bdrv_runtime_opts = {
> +    .name = "bdrv_common",
> +    .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "node-name",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Node name of the block device node",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>   /*
>    * Common part for opening disk images and files
>    *
> @@ -886,6 +900,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>       int ret, open_flags;
>       const char *filename;
>       const char *node_name = NULL;
> +    QemuOpts *opts;
>       Error *local_err = NULL;
>   
>       assert(drv != NULL);
> @@ -906,19 +921,28 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>   
>       trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
>   
> -    node_name = qdict_get_try_str(options, "node-name");
> +    opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto fail_opts;
> +    }
> +
> +    node_name = qemu_opt_get(opts, "node-name");
>       bdrv_assign_node_name(bs, node_name, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto fail_opts;
>       }
> -    qdict_del(options, "node-name");
>   
>       /* bdrv_open() with directly using a protocol as drv. This layer is already
>        * opened, so assign it to bs (while file becomes a closed 
> BlockDriverState)
>        * and return immediately. */
>       if (file != NULL && drv->bdrv_file_open) {
>           bdrv_swap(file, bs);
> +        qemu_opts_del(opts);
>           return 0;
>       }
>   
> @@ -936,7 +960,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>                           ? "Driver '%s' can only be used for read-only devices"
>                           : "Driver '%s' is not whitelisted",
>                      drv->format_name);
> -        return -ENOTSUP;
> +        ret = -ENOTSUP;
> +        goto fail_opts;
>       }
>   
>       assert(bs->copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
> @@ -945,7 +970,8 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>               bdrv_enable_copy_on_read(bs);
>           } else {
>               error_setg(errp, "Can't use copy-on-read on read-only device");
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto fail_opts;
>           }
>       }
>   
> @@ -958,7 +984,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>   
>       bs->drv = drv;
>       bs->opaque = g_malloc0(drv->instance_size);
> -
>       bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
>   
>       /* Open the image, either directly or using a protocol */

I know it's not even your patch originally, but I still don't like 
spurious empty line removals, whoever might be responsible for them.

> @@ -1010,6 +1035,8 @@ free_and_fail:
>       g_free(bs->opaque);
>       bs->opaque = NULL;
>       bs->drv = NULL;
> +fail_opts:
> +    qemu_opts_del(opts);
>       return ret;
>   }
>   

With that empty line removal hunk removed:

Reviewed-by: Max Reitz <address@hidden>