Re: [Qemu-devel] [PATCH v2 11/11] block: Remove BB interface from blockdev-add/del
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
> ---
> 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 @@
> # blo
[Qemu-devel] [PATCH v2 11/11] block: Remove BB interface from blockdev-add/del
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.
Signed-off-by: Kevin Wolf
---
blockdev.c| 127 ++
docs/qmp-commands.txt | 24 +++---
qapi/block-core.json | 30 +++-
3 files changed, 49 insertions(+), 132 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 405145a..a51434c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2846,7 +2846,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
bs = bdrv_find_node(id);
if (bs) {
-qmp_x_blockdev_del(false, NULL, true, id, &local_err);
+qmp_x_blockdev_del(id, &local_err);
if (local_err) {
error_report_err(local_err);
}
@@ -3829,7 +3829,6 @@ out:
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");
+goto fail;
+}
-QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
+bs = bds_tree_init(qdict, errp);
+if (!bs) {
+goto fail;
}
+QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
+
if (bs && bdrv_key_required(bs)) {
-if (blk) {
-monitor_remove_blk(blk);
-blk_unref(blk);
-} else {
-QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
-bdrv_unref(bs);
-}
+QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
+bdrv_unref(bs);
error_setg(errp, "blockdev-add doesn't support encrypted devices");
goto fail;
}
@@ -3900,82 +3884,43 @@ fail:
visit_free(v);
}
-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));
+return;
}
-
+aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
-if (bs) {
-if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) {
-goto out;
-}
-
-if (!blk && !QTAILQ_IN_USE(bs, monitor_list)) {
-error_setg(errp, "Node %s is not owned by the monitor",
- bs->node_name);
-goto out;
-}
+if
