| Message ID | 1437414365-11881-3-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 20.07.2015 um 19:45 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. We can probably lift this restriction once Jeff's auto-generated ID patches are in. However, allowing additional things is easy, so no objection here. > Many BDS options which are not parsed by bdrv_open() (like caching) > cannot be specified for these BB-less BDS trees yet. A future patch will > remove this limitation. This makes the command mostly useless, but that's okay. We'll be working on converting flags to QDict options one by one and then it will start working. There is, however, one flag that doesn't correspond to an option or enable an additional feature that is simply missing until then. That one worries me a bit: BDRV_O_INCOMING. We should probably include it in this patch; or maybe better add another patch before this one which moves the setting of BDRV_O_INCOMING from blockdev_init() to bdrv_open_common(). > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.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(-) Kevin
On 07.09.2015 18:12, Kevin Wolf wrote: > Am 20.07.2015 um 19:45 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. > > We can probably lift this restriction once Jeff's auto-generated ID > patches are in. However, allowing additional things is easy, so no > objection here. Maybe we can lift it, but I don't know. In order to know the node-name, you'd have to do a query-named-block-nodes before blockdev-add and another one afterwards, diff it, and thus obtain the ID. But maybe that doesn't even work, since one blockdev-add operation usually adds multiple BDSs at ones. >> Many BDS options which are not parsed by bdrv_open() (like caching) >> cannot be specified for these BB-less BDS trees yet. A future patch will >> remove this limitation. > > This makes the command mostly useless, but that's okay. Well, the future patch is part of this series, so yes, it is okay. > We'll be working > on converting flags to QDict options one by one and then it will start > working. > > There is, however, one flag that doesn't correspond to an option or > enable an additional feature that is simply missing until then. That one > worries me a bit: BDRV_O_INCOMING. We should probably include it in this > patch; or maybe better add another patch before this one which moves the > setting of BDRV_O_INCOMING from blockdev_init() to bdrv_open_common(). Hm, probably so, yes. It's still missing at the end of this series, too. However, as far as I can see, O_INCOMING is set only for the root BDS, so moving it to bdrv_open_common() would probably change behavior, though maybe that'd be a bug fix, actually. Max >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.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(-) > > Kevin >
diff --git a/blockdev.c b/blockdev.c index 62a4586..644a01c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3042,17 +3042,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(). @@ -3080,14 +3075,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); + if (ret < 0) { + goto fail; + } } - if (bdrv_key_required(blk_bs(blk))) { - blk_unref(blk); + if (bs && bdrv_key_required(bs)) { + if (blk) { + blk_unref(blk); + } else { + bdrv_unref(bs); + } error_setg(errp, "blockdev-add doesn't support encrypted devices"); goto fail; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 133fa38..bc12934 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1393,9 +1393,12 @@ # # @driver: block driver name # @id: #optional id by which the new block device can be referred to. -# This is a required option on the top level of blockdev-add, and -# currently not allowed on any other level. -# @node-name: #optional the name of a block driver state node (Since 2.0) +# This option is only allowed on the top level of blockdev-add. +# A BlockBackend will be created by blockdev-add if and only if +# this option is given. +# @node-name: #optional the name of a block driver state node (Since 2.0). +# This option is required on the top level of blockdev-add if +# the @id option is not given there. # @discard: #optional discard-related options (default: ignore) # @cache: #optional cache-related options # @aio: #optional AIO backend (default: threads) @@ -1854,7 +1857,9 @@ ## # @blockdev-add: # -# Creates a new block device. +# Creates a new block device. If the @id option is given at the top level, a +# BlockBackend will be created; otherwise, @node-name is mandatory at the top +# level and no BlockBackend will be created. # # This command is still a work in progress. It doesn't support all # block drivers, it lacks a matching blockdev-del, and more. Stay diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index 8694749..af44299 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -54,7 +54,7 @@ size=128M _make_test_img $size echo -echo === Missing ID === +echo === Missing ID and node-name === echo run_qemu <<EOF diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out index c71bb3a..354e812 100644 --- a/tests/qemu-iotests/087.out +++ b/tests/qemu-iotests/087.out @@ -1,12 +1,12 @@ QA output created by 087 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 -=== Missing ID === +=== Missing ID and node-name === Testing: QMP_VERSION {"return": {}} -{"error": {"class": "GenericError", "desc": "Block device needs an ID"}} +{"error": {"class": "GenericError", "desc": "'id' and/or 'node-name' need to be specified for the root node"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN"} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}