Re: [Qemu-block] [PATCH 24/34] block: Keep "driver" in bs->options

[Max Reitz]

On 08.05.2015 19:21, Kevin Wolf wrote:
> Instead of passing a separate drv argument to bdrv_open_common(), just
> make sure that a "driver" option is set in the QDict. This also means
> that a "driver" entry is consistently present in bs->options now.
>
> This is another step towards keeping all options in the QDict (which is
> the represenation of the blockdev-add QMP command).
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   block.c | 54 ++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7c0c9db..ef39c74 100644
> --- a/block.c
> +++ b/block.c
> @@ -795,6 +795,11 @@ static QemuOptsList bdrv_runtime_opts = {
>               .type = QEMU_OPT_STRING,
>               .help = "Node name of the block device node",
>           },
> +        {
> +            .name = "driver",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Block driver to use for the node",
> +        },
>           { /* end of list */ }
>       },
>   };
> @@ -805,18 +810,31 @@ static QemuOptsList bdrv_runtime_opts = {
>    * Removes all processed options from *options.
>    */
>   static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> -    QDict *options, int flags, BlockDriver *drv, Error **errp)
> +                            QDict *options, int flags, Error **errp)
>   {
>       int ret, open_flags;
>       const char *filename;
> +    const char *driver_name = NULL;
>       const char *node_name = NULL;
>       QemuOpts *opts;
> +    BlockDriver *drv;
>       Error *local_err = NULL;
>   
> -    assert(drv != NULL);
>       assert(bs->file == NULL);
>       assert(options != NULL && bs->options != options);
>   
> +    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;
> +    }
> +
> +    driver_name = qemu_opt_get(opts, "driver");
> +    drv = bdrv_find_format(driver_name);
> +    assert(drv != NULL);
> +
>       if (file != NULL) {
>           filename = file->filename;
>       } else {

The "return -EINVAL" in "if (drv->bdrv_needs_filename && !filename) {" 
should be changed to "ret = -EINVAL; goto fail_opts;" now.

With that changed:

Reviewed-by: Max Reitz <address@hidden>

> @@ -831,14 +849,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>   
>       trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_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) {
> @@ -1405,12 +1415,15 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>           goto fail;
>       }
>   
> +    bs->open_flags = flags;
> +    bs->options = options;
> +    options = qdict_clone_shallow(options);
> +
>       /* Find the right image format driver */
>       drv = NULL;
>       drvname = qdict_get_try_str(options, "driver");
>       if (drvname) {
>           drv = bdrv_find_format(drvname);
> -        qdict_del(options, "driver");
>           if (!drv) {
>               error_setg(errp, "Unknown driver: '%s'", drvname);
>               ret = -EINVAL;
> @@ -1425,10 +1438,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>           flags &= ~BDRV_O_PROTOCOL;
>       }
>   
> -    bs->open_flags = flags;
> -    bs->options = options;
> -    options = qdict_clone_shallow(options);
> -
>       /* Open image file without format layer */
>       if ((flags & BDRV_O_PROTOCOL) == 0) {
>           if (flags & BDRV_O_RDWR) {
> @@ -1455,6 +1464,19 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>           if (ret < 0) {
>               goto fail;
>           }
> +        /*
> +         * This option update would logically belong in bdrv_fill_options(),
> +         * but we first need to open bs->file for the probing to work, while
> +         * opening bs->file already requires the (mostly) final set of options
> +         * so that cache mode etc. can be inherited.
> +         *
> +         * Adding the driver later is somewhat ugly, but it's not an option
> +         * that would ever be inherited, so it's correct. We just need to make
> +         * sure to update both bs->options (which has the full effective
> +         * options for bs) and options (which has file.* already removed).
> +         */
> +        qdict_put(bs->options, "driver", qstring_from_str(drv->format_name));
> +        qdict_put(options, "driver", qstring_from_str(drv->format_name));
>       } else if (!drv) {
>           error_setg(errp, "Must specify either driver or file");
>           ret = -EINVAL;
> @@ -1462,7 +1484,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>       }
>   
>       /* Open the image */
> -    ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
> +    ret = bdrv_open_common(bs, file, options, flags, &local_err);
>       if (ret < 0) {
>           goto fail;
>       }