Re: [Qemu-devel] [PATCH v2 01/37] blockdev: Allow creation of BDS trees without BB

[Kevin Wolf]

Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
> If the "id" field is missing from the options given to blockdev-add,
> just omit the BlockBackend and create the BlockDriverState tree alone.
> 
> However, if "id" is missing, "node-name" must be specified; otherwise,
> the BDS tree would no longer be accessible.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  blockdev.c                 | 44 +++++++++++++++++++++++++++++++-------------
>  qapi/block-core.json       | 13 +++++++++----
>  tests/qemu-iotests/087     |  2 +-
>  tests/qemu-iotests/087.out |  4 ++--
>  4 files changed, 43 insertions(+), 20 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 6eedcf5..6d67c80 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2822,17 +2822,12 @@ out:
>  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  {
>      QmpOutputVisitor *ov = qmp_output_visitor_new();
> -    BlockBackend *blk;
> +    BlockDriverState *bs;
> +    BlockBackend *blk = NULL;
>      QObject *obj;
>      QDict *qdict;
>      Error *local_err = NULL;
>  
> -    /* Require an ID in the top level */
> -    if (!options->has_id) {
> -        error_setg(errp, "Block device needs an ID");
> -        goto fail;
> -    }
> -
>      /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with
>       * cache.direct=false instead of silently switching to aio=threads, 
> except
>       * when called from drive_new().
> @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
> **errp)
>  
>      qdict_flatten(qdict);
>  
> -    blk = blockdev_init(NULL, qdict, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto fail;
> +    if (options->has_id) {
> +        blk = blockdev_init(NULL, qdict, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +
> +        bs = blk_bs(blk);
> +    } else {
> +        int ret;
> +
> +        if (!qdict_get_try_str(qdict, "node-name")) {
> +            error_setg(errp, "'id' and/or 'node-name' need to be specified 
> for "
> +                       "the root node");
> +            goto fail;
> +        }
> +
> +        bs = NULL;
> +        ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | 
> BDRV_O_CACHE_WB,
> +                        NULL, errp);

Now all the qdict entries that aren't parsed by bdrv_open() but
converted into flags by blockdev_init() are broken if you don't give an
id. This includes read-only, discard, the cache options - in other
words, enough to make this "support" completely useless.

I guess I need to dig out my cache mode series and get it ready to be
merged. Once this is in, the parsing of the other necessary options
should be easy to move into bdrv_open().

Unless you have a good reason why we should merge this patch in its
hardly functional state now, I would consider those conversions a
dependency of this one.

Kevin