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 <kw...@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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature