diff mbox

[v2,11/11] block: Remove BB interface from blockdev-add/del

Message ID 1474376602-9665-12-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Sept. 20, 2016, 1:03 p.m. UTC
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 <kwolf@redhat.com>
---
 blockdev.c            | 127 ++++++++++++++------------------------------------
 docs/qmp-commands.txt |  24 +++-------
 qapi/block-core.json  |  30 +++---------
 3 files changed, 49 insertions(+), 132 deletions(-)

Comments

Eric Blake Sept. 20, 2016, 9:59 p.m. UTC | #1
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 <kwolf@redhat.com>
> ---
>  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.
diff mbox

Patch

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 (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) {
+        goto out;
+    }
 
-        if (bs->refcnt > 1) {
-            error_setg(errp, "Block device %s is in use",
-                       bdrv_get_device_or_node_name(bs));
-            goto out;
-        }
+    if (!bs->monitor_list.tqe_prev) {
+        error_setg(errp, "Node %s is not owned by the monitor",
+                   bs->node_name);
+        goto out;
     }
 
-    if (blk) {
-        monitor_remove_blk(blk);
-        blk_unref(blk);
-    } else {
-        QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
-        bdrv_unref(bs);
+    if (bs->refcnt > 1) {
+        error_setg(errp, "Block device %s is in use",
+                   bdrv_get_device_or_node_name(bs));
+        goto out;
     }
 
+    QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
+    bdrv_unref(bs);
+
 out:
     aio_context_release(aio_context);
 }
diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index 41f5698..860251a 100644
--- a/docs/qmp-commands.txt
+++ 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
+otherwise being used.
 
 This command is still a work in progress and is considered
 experimental. Stay away from it unless you want to help with its
@@ -3187,8 +3178,7 @@  development.
 
 Arguments:
 
-- "id": Name of the block backend device to delete (json-string, optional)
-- "node-name": Name of the graph node to delete (json-string, optional)
+- "node-name": Name of the graph node to delete (json-string)
 
 Example:
 
@@ -3196,7 +3186,7 @@  Example:
      "arguments": {
          "options": {
              "driver": "qcow2",
-             "id": "drive0",
+             "node-name": "node0",
              "file": {
                  "driver": "file",
                  "filename": "test.qcow2"
@@ -3208,7 +3198,7 @@  Example:
 <- { "return": {} }
 
 -> { "execute": "x-blockdev-del",
-     "arguments": { "id": "drive0" }
+     "arguments": { "node-name": "node0" }
    }
 <- { "return": {} }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5f04dab..46661e6 100644
--- a/qapi/block-core.json
+++ 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.
-# @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.
 #
 # This command is still a work in progress and is considered
 # experimental. Stay away from it unless you want to help with its
 # development.
 #
-# @id: #optional Name of the block backend device to delete.
-#
-# @node-name: #optional Name of the graph node to delete.
+# @node-name: Name of the graph node to delete.
 #
 # Since: 2.5
 ##
-{ 'command': 'x-blockdev-del', 'data': { '*id': 'str', '*node-name': 'str' } }
+{ 'command': 'x-blockdev-del', 'data': { 'node-name': 'str' } }
 
 ##
 # @blockdev-open-tray: