[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 11/11] block: Remove BB interface from blockd
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 11/11] block: Remove BB interface from blockdev-add/del |
Date: |
Tue, 20 Sep 2016 16:59:47 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
On 09/20/2016 08:03 AM, Kevin Wolf wrote:
> With this patch, blockdev-add always works on a node level, i.e. it
> creates a BDS, but no BB. Consequently, x-blockdev-del doesn't need the
> 'device' option any more, but 'node-name' becomes mandatory.
How close are we to promoting x-blockdev-del out of the x- namespace? Do
we "just" need the remaining blockdev devices exposed in QMP?
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> blockdev.c | 127
> ++++++++++++++------------------------------------
> docs/qmp-commands.txt | 24 +++-------
> qapi/block-core.json | 30 +++---------
> 3 files changed, 49 insertions(+), 132 deletions(-)
>
> void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> {
> BlockDriverState *bs;
> - BlockBackend *blk = NULL;
> QObject *obj;
> Visitor *v = qmp_output_visitor_new(&obj);
> QDict *qdict;
> @@ -3861,37 +3860,22 @@ void qmp_blockdev_add(BlockdevOptions *options, Error
> **errp)
>
> qdict_flatten(qdict);
>
> - 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 {
> - 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 = bds_tree_init(qdict, errp);
> - if (!bs) {
> - goto fail;
> - }
> + if (!qdict_get_try_str(qdict, "node-name")) {
> + error_setg(errp, "'id' and/or 'node-name' need to be specified for "
> + "the root node");
Umm, 'id' can't be specified, after this change :)
> -void qmp_x_blockdev_del(bool has_id, const char *id,
> - bool has_node_name, const char *node_name, Error
> **errp)
> +void qmp_x_blockdev_del(const char *node_name, Error **errp)
> {
> AioContext *aio_context;
> - BlockBackend *blk;
> BlockDriverState *bs;
>
> - if (has_id && has_node_name) {
> - error_setg(errp, "Only one of id and node-name must be specified");
> - return;
> - } else if (!has_id && !has_node_name) {
> - error_setg(errp, "No block device specified");
> + bs = bdrv_find_node(node_name);
> + if (!bs) {
> + error_setg(errp, "Cannot find node %s", node_name);
> return;
> }
> -
> - if (has_id) {
> - /* blk_by_name() never returns a BB that is not owned by the monitor
> */
> - blk = blk_by_name(id);
> - if (!blk) {
> - error_setg(errp, "Cannot find block backend %s", id);
> - return;
> - }
> - if (blk_legacy_dinfo(blk)) {
> - error_setg(errp, "Deleting block backend added with drive-add"
> - " is not supported");
> - return;
> - }
> - if (blk_get_refcnt(blk) > 1) {
> - error_setg(errp, "Block backend %s is in use", id);
> - return;
> - }
> - bs = blk_bs(blk);
> - aio_context = blk_get_aio_context(blk);
> - } else {
> - blk = NULL;
> - bs = bdrv_find_node(node_name);
> - if (!bs) {
> - error_setg(errp, "Cannot find node %s", node_name);
> - return;
> - }
> - if (bdrv_has_blk(bs)) {
> - error_setg(errp, "Node %s is in use by %s",
> - node_name, bdrv_get_parent_name(bs));
> - return;
> - }
> - aio_context = bdrv_get_aio_context(bs);
> + if (bdrv_has_blk(bs)) {
> + error_setg(errp, "Node %s is in use by %s",
> + node_name, bdrv_get_parent_name(bs));
Is this the source of the "(null)" I was asking about in 10/11?
> +++ b/docs/qmp-commands.txt
> @@ -3141,7 +3141,7 @@ Example (2):
> "arguments": {
> "options": {
> "driver": "qcow2",
> - "id": "my_disk",
> + "node-name": "my_disk",
> "discard": "unmap",
> "cache": {
> "direct": true,
> @@ -3168,18 +3168,9 @@ x-blockdev-del
> ------------
> Since 2.5
>
> -Deletes a block device thas has been added using blockdev-add.
> -The selected device can be either a block backend or a graph node.
> -
> -In the former case the backend will be destroyed, along with its
> -inserted medium if there's any. The command will fail if the backend
> -or its medium are in use.
> -
> -In the latter case the node will be destroyed. The command will fail
> -if the node is attached to a block backend or is otherwise being
> -used.
> -
> -One of "id" or "node-name" must be specified, but not both.
> +Deletes a block device that has been added using blockdev-add.
> +The command will fail # if the node is attached to a device or is
Spurious #.
> +++ b/qapi/block-core.json
> @@ -2217,13 +2217,8 @@
> # block devices, independent of the block driver:
> #
> # @driver: block driver name
> -# @id: #optional id by which the new block device can be referred
> to.
> -# 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.
Removing an option. Not backward-compatible, but we've been warning
users that blockdev-add is not ready for primetime yet, so I'm okay with
it (and at least introspection shows it).
> -# @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.
> +# @node-name: #optional the node name of the new node (Since 2.0).
> +# This option is required on the top level of blockdev-add.
> # @discard: #optional discard-related options (default: ignore)
> # @cache: #optional cache-related options
> # @aio: #optional AIO backend (default: threads)
> @@ -2238,8 +2233,6 @@
> ##
> { 'union': 'BlockdevOptions',
> 'base': { 'driver': 'BlockdevDriver',
> -# TODO 'id' is a BB-level option, remove it
> - '*id': 'str',
> '*node-name': 'str',
> '*discard': 'BlockdevDiscardOptions',
> '*cache': 'BlockdevCacheOptions',
> @@ -2323,29 +2316,18 @@
> # @x-blockdev-del:
> #
> # Deletes a block device that has been added using blockdev-add.
> -# The selected device can be either a block backend or a graph node.
> -#
> -# In the former case the backend will be destroyed, along with its
> -# inserted medium if there's any. The command will fail if the backend
> -# or its medium are in use.
> -#
> -# In the latter case the node will be destroyed. The command will fail
> -# if the node is attached to a block backend or is otherwise being
> -# used.
> -#
> -# One of @id or @node-name must be specified, but not both.
> +# The command will fail # if the node is attached to a device or is
> +# otherwise being used.
Spurious # (copy-and-paste strikes again)
Overall, I like the result, but I think it needs just a bit of polish
before I give R-b.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v2 02/11] qemu-iotests/067: Avoid blockdev-add with id, (continued)
- [Qemu-devel] [PATCH v2 03/11] qemu-iotests/071: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20
- [Qemu-devel] [PATCH v2 05/11] qemu-iotests/087: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20
- [Qemu-devel] [PATCH v2 06/11] qemu-iotests/117: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20
- [Qemu-devel] [PATCH v2 08/11] qemu-iotests/124: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20
- [Qemu-devel] [PATCH v2 11/11] block: Remove BB interface from blockdev-add/del, Kevin Wolf, 2016/09/20
- Re: [Qemu-devel] [PATCH v2 11/11] block: Remove BB interface from blockdev-add/del,
Eric Blake <=
- [Qemu-devel] [PATCH v2 10/11] qemu-iotests/141: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20
- [Qemu-devel] [PATCH v2 07/11] qemu-iotests/118: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20
- [Qemu-devel] [PATCH v2 09/11] qemu-iotests/139: Avoid blockdev-add with id, Kevin Wolf, 2016/09/20