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

[Max Reitz <mreitz@redhat.com>]

On 2015-03-04 at 08:39, Kevin Wolf wrote:
> 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 <mreitz@redhat.com>
>> ---
>>   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.

See patch 23.

Max

>
> 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