[PATCH v10 0/8] blockdev-replace
Hi all! This series presents a new command blockdev-replace, which helps to insert/remove filters anywhere in the block graph. It can: - replace qdev block-node by qdev-id - replace export block-node by export-id - replace any child of parent block-node by node-name and child name So insertions is done in two steps: 1. blockdev_add (create filter node, unparented) [some parent] [new filter] | | V V [ some child ] 2. blockdev-replace (replace child by the filter) [some parent] | V [new filter] | V [some child] And removal is done in reverse order: 1. blockdev-replace (go back to picture 1) 2. blockdev_del (remove filter node) Ideally, we should do both operations (add + replace or replace + del) in a transaction, but that should be another series. v10: reabsed on master. 02: change exp-blk check into assert 04: rewrite to simpler API (on unique parent id) 07: switch to new API 08: new Vladimir Sementsov-Ogievskiy (8): block-backend: blk_root(): drop const specifier on return type block/export: add blk_by_export_id() block: make bdrv_find_child() function public qapi: add blockdev-replace command block: bdrv_get_xdbg_block_graph(): report export ids iotests.py: introduce VM.assert_edges_list() method iotests: add filter-insertion deprecate names duplication between qdev, block-node and block-export block.c | 29 +++ block/block-backend.c | 2 +- block/export/export.c | 41 ++++ blockdev.c | 82 +++++-- docs/about/deprecated.rst | 10 + include/block/block-global-state.h | 1 + include/block/block_int-io.h | 2 + include/block/export.h | 1 + include/system/block-backend-global-state.h | 5 +- qapi/block-core.json | 24 ++ stubs/blk-by-qdev-id.c | 18 ++ stubs/blk-exp-find-by-blk.c | 13 + stubs/meson.build | 2 + system/qdev-monitor.c | 16 ++ tests/qemu-iotests/iotests.py | 17 ++ tests/qemu-iotests/tests/filter-insertion | 222 ++++++++++++++++++ tests/qemu-iotests/tests/filter-insertion.out | 5 + 17 files changed, 474 insertions(+), 16 deletions(-) create mode 100644 stubs/blk-by-qdev-id.c create mode 100644 stubs/blk-exp-find-by-blk.c create mode 100755 tests/qemu-iotests/tests/filter-insertion create mode 100644 tests/qemu-iotests/tests/filter-insertion.out -- 2.52.0
We'll need get non-const child pointer for graph modifications in further commits. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block/block-backend.c | 2 +- include/system/block-backend-global-state.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 9944657120..d4b48ac3b0 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2834,7 +2834,7 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, bytes, read_flags, write_flags); } -const BdrvChild *blk_root(BlockBackend *blk) +BdrvChild *blk_root(BlockBackend *blk) { GLOBAL_STATE_CODE(); return blk->root; diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index c3849640df..a438f21dff 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -117,7 +117,7 @@ void blk_set_force_allow_inactivate(BlockBackend *blk); bool blk_register_buf(BlockBackend *blk, void *host, size_t size, Error **errp); void blk_unregister_buf(BlockBackend *blk, void *host, size_t size); -const BdrvChild *blk_root(BlockBackend *blk); +BdrvChild *blk_root(BlockBackend *blk); int blk_make_empty(BlockBackend *blk, Error **errp); -- 2.52.0
We need it for further blockdev-replace functionality. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block/export/export.c | 15 +++++++++++++++ include/system/block-backend-global-state.h | 1 + 2 files changed, 16 insertions(+) diff --git a/block/export/export.c b/block/export/export.c index f3bbf11070..d11beb900f 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -370,3 +370,18 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp) return head; } + +BlockBackend *blk_by_export_id(const char *id, Error **errp) +{ + BlockExport *exp; + + exp = blk_exp_find(id); + if (exp == NULL) { + error_setg(errp, "Export '%s' not found", id); + return NULL; + } + + assert(exp->blk); + + return exp->blk; +} diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index a438f21dff..f23b9f1518 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -72,6 +72,7 @@ void blk_detach_dev(BlockBackend *blk, DeviceState *dev); DeviceState *blk_get_attached_dev(BlockBackend *blk); BlockBackend *blk_by_dev(void *dev); BlockBackend *blk_by_qdev_id(const char *id, Error **errp); +BlockBackend *blk_by_export_id(const char *id, Error **errp); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags); -- 2.52.0
To be reused soon for blockdev-replace functionality. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block.c | 13 +++++++++++++ blockdev.c | 14 -------------- include/block/block_int-io.h | 2 ++ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 48a17f393c..b13d06f709 100644 --- a/block.c +++ b/block.c @@ -8288,6 +8288,19 @@ int bdrv_make_empty(BdrvChild *c, Error **errp) return 0; } +BdrvChild *bdrv_find_child(BlockDriverState *parent_bs, const char *child_name) +{ + BdrvChild *child; + + QLIST_FOREACH(child, &parent_bs->children, next) { + if (strcmp(child->name, child_name) == 0) { + return child; + } + } + + return NULL; +} + /* * Return the child that @bs acts as an overlay for, and from which data may be * copied in COW or COR operations. Usually this is the backing file. diff --git a/blockdev.c b/blockdev.c index 6e86c6262f..2540f991b3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3569,20 +3569,6 @@ unlock: } } -static BdrvChild * GRAPH_RDLOCK -bdrv_find_child(BlockDriverState *parent_bs, const char *child_name) -{ - BdrvChild *child; - - QLIST_FOREACH(child, &parent_bs->children, next) { - if (strcmp(child->name, child_name) == 0) { - return child; - } - } - - return NULL; -} - void qmp_x_blockdev_change(const char *parent, const char *child, const char *node, Error **errp) { diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h index ed8b5657d6..8ff3bc6d87 100644 --- a/include/block/block_int-io.h +++ b/include/block/block_int-io.h @@ -130,6 +130,8 @@ bdrv_co_refresh_total_sectors(BlockDriverState *bs, int64_t hint); int co_wrapper_mixed_bdrv_rdlock bdrv_refresh_total_sectors(BlockDriverState *bs, int64_t hint); +BdrvChild * GRAPH_RDLOCK +bdrv_find_child(BlockDriverState *parent_bs, const char *child_name); BdrvChild * GRAPH_RDLOCK bdrv_cow_child(BlockDriverState *bs); BdrvChild * GRAPH_RDLOCK bdrv_filter_child(BlockDriverState *bs); BdrvChild * GRAPH_RDLOCK bdrv_filter_or_cow_child(BlockDriverState *bs); -- 2.52.0
Add a command that can replace bs in following BdrvChild structures: - qdev blk root child - block-export blk root child - any child of BlockDriverState selected by child-name Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- blockdev.c | 68 ++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 24 +++++++++++++++ stubs/blk-by-qdev-id.c | 13 ++++++++ stubs/meson.build | 1 + 4 files changed, 106 insertions(+) create mode 100644 stubs/blk-by-qdev-id.c diff --git a/blockdev.c b/blockdev.c index 2540f991b3..3082a5763c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3681,6 +3681,74 @@ out: bdrv_drain_all_end(); } +void qmp_blockdev_replace(const char *parent, const char *child, + const char *new_child, Error **errp) +{ + BdrvChild *child_to_replace = NULL; + BlockDriverState *new_child_bs; + Error *dev_err = NULL, *exp_err = NULL; + BlockDriverState *parent_bs = bdrv_find_node(parent); + BlockBackend *dev_blk = blk_by_qdev_id(parent, &dev_err); + BlockBackend *exp_blk = blk_by_export_id(parent, &exp_err); + unsigned found = !!parent_bs + !!dev_blk + !!exp_blk; + + if (found == 0) { + error_setg(errp, "Neither device, nor export, nor block-node exist" + " with name '%s'. block-node: not found," + " device block-backend: %s, export block-backend: %s", + parent, error_get_pretty(dev_err), + error_get_pretty(exp_err)); + } + error_free(dev_err); + error_free(exp_err); + + if (found == 0) { + return; + } + + if (found > 1) { + error_setg(errp, "Parent name '%s' is ambigous: block-node %s," + " device block-backend %s, export block-backend %s", + parent, parent_bs ? "found" : "not found", + dev_blk ? "found" : "not found", + exp_blk ? "found" : "not found"); + return; + } + + if (parent_bs) { + child_to_replace = bdrv_find_child(parent_bs, child); + if (!child_to_replace) { + error_setg(errp, "Block driver node '%s' doesn't have child " + "named '%s'", parent, child); + return; + } + } else { + BlockBackend *blk = dev_blk ?: exp_blk; + + if (strcmp(child, "root")) { + error_setg(errp, "Devices and exports may have only 'root' child"); + } + + child_to_replace = blk_root(blk); + if (!child_to_replace) { + error_setg(errp, "%s '%s' is empty, nothing to replace", + dev_blk ? "Device" : "Export", parent); + return; + } + } + + assert(child_to_replace); + assert(child_to_replace->bs); + + new_child_bs = bdrv_find_node(new_child); + if (!new_child_bs) { + error_setg(errp, "Node '%s' not found", new_child); + return; + } + + bdrv_replace_child_bs(child_to_replace, new_child_bs, errp); +} + QemuOptsList qemu_common_drive_opts = { .name = "drive", .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head), diff --git a/qapi/block-core.json b/qapi/block-core.json index b82af74256..9cc7c3d140 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -6334,3 +6334,27 @@ ## { 'struct': 'DummyBlockCoreForceArrays', 'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } } + +## +# @blockdev-replace: +# +# Replace a block-node associated with device (@parent should be +# QOM path, and @child should be "root") or with block-export (@parent +# should be export name, and @child should be "root") or any child +# of block-node (@parent should be node-name, and @child should be any +# if its children names) with @new-child block-node. +# +# @parent: QOM path or block-export or node-name, which @child should +# be repalced. If several matching parents exist, the command +# will fail +# +# @child: child to replace. Must be "root" when parent is QOM path or +# block-export +# +# @new-child: node-name of the block-node, which should become a +# replacement for @child's current block-node +# +# Since 11.0 +## +{ 'command': 'blockdev-replace', + 'data': { 'parent': 'str', 'child': 'str', 'new-child': 'str' } } diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c new file mode 100644 index 0000000000..66ead77f4d --- /dev/null +++ b/stubs/blk-by-qdev-id.c @@ -0,0 +1,13 @@ +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "system/block-backend.h" + +BlockBackend *blk_by_qdev_id(const char *id, Error **errp) +{ + /* + * We expect this when blockdev-change is called with parent-type=qdev, + * but qdev-monitor is not linked in. So no blk_ API is not available. + */ + error_setg(errp, "Parameter 'parent-type' does not accept value 'qdev'"); + return NULL; +} diff --git a/stubs/meson.build b/stubs/meson.build index d3b551f4de..93a52d273e 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -15,6 +15,7 @@ if have_block stub_ss.add(files('bdrv-next-monitor-owned.c')) stub_ss.add(files('blk-commit-all.c')) stub_ss.add(files('blk-exp-close-all.c')) + stub_ss.add(files('blk-by-qdev-id.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) stub_ss.add(files('get-vm-name.c')) -- 2.52.0
Currently for block exports we report empty blk names. That's not good. Let's try to find corresponding block export and report its id. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block.c | 4 ++++ block/export/export.c | 13 +++++++++++++ include/block/export.h | 1 + stubs/blk-exp-find-by-blk.c | 9 +++++++++ stubs/meson.build | 1 + 5 files changed, 28 insertions(+) create mode 100644 stubs/blk-exp-find-by-blk.c diff --git a/block.c b/block.c index b13d06f709..8254d57212 100644 --- a/block.c +++ b/block.c @@ -6354,7 +6354,11 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp) for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { char *allocated_name = NULL; const char *name = blk_name(blk); + BlockExport *exp = blk_exp_find_by_blk(blk); + if (!*name && exp) { + name = exp->id; + } if (!*name) { name = allocated_name = blk_get_attached_dev_id(blk); } diff --git a/block/export/export.c b/block/export/export.c index d11beb900f..9169b43e13 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -60,6 +60,19 @@ BlockExport *blk_exp_find(const char *id) return NULL; } +BlockExport *blk_exp_find_by_blk(BlockBackend *blk) +{ + BlockExport *exp; + + QLIST_FOREACH(exp, &block_exports, next) { + if (exp->blk == blk) { + return exp; + } + } + + return NULL; +} + static const BlockExportDriver *blk_exp_find_driver(BlockExportType type) { int i; diff --git a/include/block/export.h b/include/block/export.h index 4bd9531d4d..2d3a0b4a28 100644 --- a/include/block/export.h +++ b/include/block/export.h @@ -85,6 +85,7 @@ struct BlockExport { BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp); BlockExport *blk_exp_find(const char *id); +BlockExport *blk_exp_find_by_blk(BlockBackend *blk); void blk_exp_ref(BlockExport *exp); void blk_exp_unref(BlockExport *exp); void blk_exp_request_shutdown(BlockExport *exp); diff --git a/stubs/blk-exp-find-by-blk.c b/stubs/blk-exp-find-by-blk.c new file mode 100644 index 0000000000..20f7ff1bdd --- /dev/null +++ b/stubs/blk-exp-find-by-blk.c @@ -0,0 +1,9 @@ +#include "qemu/osdep.h" +#include "system/block-backend.h" +#include "block/export.h" + +BlockExport *blk_exp_find_by_blk(BlockBackend *blk) +{ + return NULL; +} + diff --git a/stubs/meson.build b/stubs/meson.build index 93a52d273e..d530a637d9 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -16,6 +16,7 @@ if have_block stub_ss.add(files('blk-commit-all.c')) stub_ss.add(files('blk-exp-close-all.c')) stub_ss.add(files('blk-by-qdev-id.c')) + stub_ss.add(files('blk-exp-find-by-blk.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) stub_ss.add(files('get-vm-name.c')) -- 2.52.0
Add an alternative method to check block graph, to be used in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- tests/qemu-iotests/iotests.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 05274772ce..d6f4e890da 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -1126,6 +1126,23 @@ def check_bitmap_status(self, node_name, bitmap_name, fields): return fields.items() <= ret.items() + def get_block_graph(self): + """ + Returns block graph in form of edges list, where each edge is a tuple: + (parent_node_name, child_name, child_node_name) + """ + graph = self.qmp('x-debug-query-block-graph')['return'] + + nodes = {n['id']: n['name'] for n in graph['nodes']} + # Check that all names are unique: + assert len(set(nodes.values())) == len(nodes) + + return [(nodes[e['parent']], e['name'], nodes[e['child']]) + for e in graph['edges']] + + def assert_edges_list(self, edges): + assert sorted(edges) == sorted(self.get_block_graph()) + def assert_block_path(self, root, path, expected_node, graph=None): """ Check whether the node under the given path in the block graph -- 2.52.0
Demonstrate new blockdev-replace API for filter insertion and removal. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- tests/qemu-iotests/tests/filter-insertion | 222 ++++++++++++++++++ tests/qemu-iotests/tests/filter-insertion.out | 5 + 2 files changed, 227 insertions(+) create mode 100755 tests/qemu-iotests/tests/filter-insertion create mode 100644 tests/qemu-iotests/tests/filter-insertion.out diff --git a/tests/qemu-iotests/tests/filter-insertion b/tests/qemu-iotests/tests/filter-insertion new file mode 100755 index 0000000000..23e114f959 --- /dev/null +++ b/tests/qemu-iotests/tests/filter-insertion @@ -0,0 +1,222 @@ +#!/usr/bin/env python3 +# +# Tests for inserting and removing filters in a block graph. +# +# Copyright (c) 2022 Virtuozzo International GmbH. +# +# SPDX-License-Identifier: GPL-2.0-or-later + +import os + +import iotests +from iotests import qemu_img_create, try_remove + + +disk = os.path.join(iotests.test_dir, 'disk') +sock = os.path.join(iotests.sock_dir, 'sock') +size = 1024 * 1024 + + +class TestFilterInsertion(iotests.QMPTestCase): + def setUp(self): + qemu_img_create('-f', iotests.imgfmt, disk, str(size)) + + self.vm = iotests.VM() + self.vm.launch() + + self.vm.cmd('blockdev-add', { + 'node-name': 'disk0', + 'driver': 'qcow2', + 'file': { + 'node-name': 'file0', + 'driver': 'file', + 'filename': disk + } + }) + + def tearDown(self): + self.vm.shutdown() + os.remove(disk) + try_remove(sock) + + def test_simple_insertion(self): + vm = self.vm + + vm.cmd('blockdev-add', { + 'node-name': 'filter', + 'driver': 'blkdebug', + 'image': 'file0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'disk0', + 'child': 'file', + 'new-child': 'filter' + }) + + # Filter inserted: + # disk0 -file-> filter -file-> file0 + vm.assert_edges_list([ + ('disk0', 'file', 'filter'), + ('filter', 'image', 'file0') + ]) + + vm.cmd('blockdev-replace', { + 'parent': 'disk0', + 'child': 'file', + 'new-child': 'file0' + }) + + # Filter replaced, but still exists: + # dik0 -file-> file0 <-file- filter + vm.assert_edges_list([ + ('disk0', 'file', 'file0'), + ('filter', 'image', 'file0') + ]) + + vm.cmd('blockdev-del', node_name='filter') + + # Filter removed + # dik0 -file-> file0 + vm.assert_edges_list([ + ('disk0', 'file', 'file0') + ]) + + def test_insert_under_qdev(self): + vm = self.vm + + vm.cmd('device_add', driver='virtio-scsi') + vm.cmd('device_add', id='sda', driver='scsi-hd', + drive='disk0') + + vm.cmd('blockdev-add', { + 'node-name': 'filter', + 'driver': 'compress', + 'file': 'disk0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'sda', + 'child': 'root', + 'new-child': 'filter' + }) + + # Filter inserted: + # sda -root-> filter -file-> disk0 -file-> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('sda', 'root', 'filter'), + ('filter', 'file', 'disk0'), + ('disk0', 'file', 'file0'), + ]) + + vm.cmd('blockdev-replace', { + 'parent': 'sda', + 'child': 'root', + 'new-child': 'disk0' + }) + vm.cmd('blockdev-del', node_name='filter') + + # Filter removed: + # sda -root-> disk0 -file-> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('sda', 'root', 'disk0'), + ('disk0', 'file', 'file0'), + ]) + + def test_insert_under_nbd_export(self): + vm = self.vm + + vm.cmd('nbd-server-start', + addr={'type': 'unix', 'data': {'path': sock}}) + vm.cmd('block-export-add', id='exp1', type='nbd', + node_name='disk0', name='exp1') + vm.cmd('block-export-add', id='exp2', type='nbd', + node_name='disk0', name='exp2') + vm.cmd('object-add', qom_type='throttle-group', + id='tg', limits={'iops-read': 1}) + + vm.cmd('blockdev-add', { + 'node-name': 'filter', + 'driver': 'throttle', + 'throttle-group': 'tg', + 'file': 'disk0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'exp1', + 'child': 'root', + 'new-child': 'filter' + }) + + # Only exp1 is throttled, exp2 is not: + # exp1 -root-> filter + # | + # |file + # v + # exp2 -file-> disk0 -file> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('exp1', 'root', 'filter'), + ('filter', 'file', 'disk0'), + ('disk0', 'file', 'file0'), + ('exp2', 'root', 'disk0') + ]) + + vm.cmd('blockdev-replace', { + 'parent': 'exp2', + 'child': 'root', + 'new-child': 'filter' + }) + + # Both throttled: + # exp1 -root-> filter <-file- exp2 + # | + # |file + # v + # disk0 -file> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('exp1', 'root', 'filter'), + ('filter', 'file', 'disk0'), + ('disk0', 'file', 'file0'), + ('exp2', 'root', 'filter') + ]) + + # Check, that filter is in use and can't be removed + result = vm.qmp('blockdev-del', node_name='filter') + self.assert_qmp(result, 'error/desc', 'Node filter is in use') + + vm.cmd('blockdev-replace', { + 'parent': 'exp1', + 'child': 'root', + 'new-child': 'disk0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'exp2', + 'child': 'root', + 'new-child': 'disk0' + }) + vm.cmd('blockdev-del', node_name='filter') + + # Filter removed: + # exp1 -root-> disk0 <-file- exp2 + # | + # |file + # v + # file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('exp1', 'root', 'disk0'), + ('disk0', 'file', 'file0'), + ('exp2', 'root', 'disk0') + ]) + + +if __name__ == '__main__': + iotests.main( + supported_fmts=['qcow2'], + supported_protocols=['file'] + ) diff --git a/tests/qemu-iotests/tests/filter-insertion.out b/tests/qemu-iotests/tests/filter-insertion.out new file mode 100644 index 0000000000..8d7e996700 --- /dev/null +++ b/tests/qemu-iotests/tests/filter-insertion.out @@ -0,0 +1,5 @@ +... +---------------------------------------------------------------------- +Ran 3 tests + +OK -- 2.52.0
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block.c | 12 ++++++++++++ block/export/export.c | 13 +++++++++++++ docs/about/deprecated.rst | 10 ++++++++++ include/block/block-global-state.h | 1 + include/system/block-backend-global-state.h | 2 ++ stubs/blk-by-qdev-id.c | 5 +++++ stubs/blk-exp-find-by-blk.c | 4 ++++ system/qdev-monitor.c | 16 ++++++++++++++++ 8 files changed, 63 insertions(+) diff --git a/block.c b/block.c index 8254d57212..5eae8b8623 100644 --- a/block.c +++ b/block.c @@ -1649,6 +1649,9 @@ static void bdrv_assign_node_name(BlockDriverState *bs, goto out; } + warn_device_exists(node_name); + warn_block_export_exists(node_name); + /* copy node name into the bs and insert it into the graph list */ pstrcpy(bs->node_name, sizeof(bs->node_name), node_name); QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list); @@ -6233,6 +6236,15 @@ BlockDriverState *bdrv_find_node(const char *node_name) return NULL; } +void warn_block_node_exists(const char *node_name) +{ + if (bdrv_find_node(node_name)) { + warn_report("block node already exist with name '%s'. " + "Ambigous identifiers are deprecated. " + "In future that would be an error.", node_name); + } +} + /* Put this QMP function here so it can access the static graph_bdrv_states. */ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp) diff --git a/block/export/export.c b/block/export/export.c index 9169b43e13..e65d1bec8e 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -23,6 +23,7 @@ #include "qapi/qapi-commands-block-export.h" #include "qapi/qapi-events-block-export.h" #include "qemu/id.h" +#include "qemu/error-report.h" #ifdef CONFIG_VHOST_USER_BLK_SERVER #include "vhost-user-blk-server.h" #endif @@ -108,6 +109,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) return NULL; } + warn_device_exists(export->id); + warn_block_node_exists(export->id); + drv = blk_exp_find_driver(export->type); if (!drv) { error_setg(errp, "No driver found for the requested export type"); @@ -384,6 +388,15 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp) return head; } +void warn_block_export_exists(const char *id) +{ + if (blk_exp_find(id)) { + warn_report("block-export already exist with name '%s'. " + "Ambigous identifiers are deprecated. " + "In future that would be an error.", id); + } +} + BlockBackend *blk_by_export_id(const char *id, Error **errp) { BlockExport *exp; diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 88efa3aa80..18bb1eeafc 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -551,3 +551,13 @@ command documentation for details on the ``fdset`` usage. The ``zero-blocks`` capability was part of the block migration which doesn't exist anymore since it was removed in QEMU v9.1. + +Identifiers +----------- + +Possibility to intersect qdev ids/paths, block node names, and block +export names namespaces is deprecated. In future that would be +abandoned and all block exports, block nodes and devices will have +unique names. Now, reusing the name for another type of object (for +example, creating block-node with node-name equal to existing qdev +id) produce a warning. diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index ed89999f0f..ea50478fc4 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -207,6 +207,7 @@ void bdrv_aio_cancel(BlockAIOCB *acb); int bdrv_has_zero_init_1(BlockDriverState *bs); int coroutine_mixed_fn GRAPH_RDLOCK bdrv_has_zero_init(BlockDriverState *bs); BlockDriverState *bdrv_find_node(const char *node_name); +void warn_block_node_exists(const char *node_name); BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp); XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp); BlockDriverState *bdrv_lookup_bs(const char *device, diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index f23b9f1518..69e6aee618 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -73,6 +73,8 @@ DeviceState *blk_get_attached_dev(BlockBackend *blk); BlockBackend *blk_by_dev(void *dev); BlockBackend *blk_by_qdev_id(const char *id, Error **errp); BlockBackend *blk_by_export_id(const char *id, Error **errp); +void warn_block_export_exists(const char *id); +void warn_device_exists(const char *id); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags); diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c index 66ead77f4d..c83a2dde0d 100644 --- a/stubs/blk-by-qdev-id.c +++ b/stubs/blk-by-qdev-id.c @@ -11,3 +11,8 @@ BlockBackend *blk_by_qdev_id(const char *id, Error **errp) error_setg(errp, "Parameter 'parent-type' does not accept value 'qdev'"); return NULL; } + +void warn_device_exists(const char *id) +{ + /* do nothing */ +} diff --git a/stubs/blk-exp-find-by-blk.c b/stubs/blk-exp-find-by-blk.c index 20f7ff1bdd..a98c4572fc 100644 --- a/stubs/blk-exp-find-by-blk.c +++ b/stubs/blk-exp-find-by-blk.c @@ -7,3 +7,7 @@ BlockExport *blk_exp_find_by_blk(BlockBackend *blk) return NULL; } +void warn_block_export_exists(const char *id) +{ + /* do nothing */ +} diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index be18902bb2..67b9da952d 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -605,6 +605,8 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) OBJECT(dev), NULL); if (prop) { dev->id = id; + warn_block_export_exists(id); + warn_block_node_exists(id); } else { error_setg(errp, "Duplicate device ID '%s'", id); g_free(id); @@ -903,6 +905,20 @@ static DeviceState *find_device_state(const char *id, bool use_generic_error, return dev; } +void warn_device_exists(const char *id) +{ + Object *obj = object_resolve_path_at(qdev_get_peripheral(), id); + + if (obj) { + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE); + + warn_report("%s '%s' already exist. " + "Ambigous identifiers are deprecated. " + "In future that would be an error.", + dev ? "Device" : "Object", id); + } +} + void qdev_unplug(DeviceState *dev, Error **errp) { HotplugHandler *hotplug_ctrl; -- 2.52.0
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- v11: rework separate warn_s into one common function check_existing_parent_id(). block.c | 6 ++++++ block/export/export.c | 6 ++++++ blockdev.c | 21 +++++++++++++++++++++ docs/about/deprecated.rst | 10 ++++++++++ include/block/block-global-state.h | 2 ++ stubs/check-existing-parent-id.c | 6 ++++++ stubs/meson.build | 1 + system/qdev-monitor.c | 20 ++++++++++++++++++++ 8 files changed, 72 insertions(+) create mode 100644 stubs/check-existing-parent-id.c diff --git a/block.c b/block.c index 8254d57212..69674ad4ed 100644 --- a/block.c +++ b/block.c @@ -1618,6 +1618,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs, { char *gen_node_name = NULL; GLOBAL_STATE_CODE(); + Error *local_err; if (!node_name) { node_name = gen_node_name = id_generate(ID_BLOCK); @@ -1649,6 +1650,11 @@ static void bdrv_assign_node_name(BlockDriverState *bs, goto out; } + if (!check_existing_parent_id(node_name, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + /* copy node name into the bs and insert it into the graph list */ pstrcpy(bs->node_name, sizeof(bs->node_name), node_name); QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list); diff --git a/block/export/export.c b/block/export/export.c index 9169b43e13..174c86b4d2 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -96,6 +96,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) AioContext *ctx; uint64_t perm; int ret; + Error *local_err = NULL; GLOBAL_STATE_CODE(); @@ -108,6 +109,11 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) return NULL; } + if (!check_existing_parent_id(export->id, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + drv = blk_exp_find_driver(export->type); if (!drv) { error_setg(errp, "No driver found for the requested export type"); diff --git a/blockdev.c b/blockdev.c index 3082a5763c..643b2132c9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3681,6 +3681,27 @@ out: bdrv_drain_all_end(); } + +bool check_existing_parent_id(const char *id, Error **errp) +{ + if (bdrv_find_node(id)) { + error_setg(errp, "block node with id '%s' already exist", id); + return false; + } + + if (blk_by_qdev_id(id, NULL)) { + error_setg(errp, "block device with id '%s' already exist", id); + return false; + } + + if (blk_by_export_id(id, NULL)) { + error_setg(errp, "block export with id '%s' already exist", id); + return false; + } + + return true; +} + void qmp_blockdev_replace(const char *parent, const char *child, const char *new_child, Error **errp) { diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 88efa3aa80..18bb1eeafc 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -551,3 +551,13 @@ command documentation for details on the ``fdset`` usage. The ``zero-blocks`` capability was part of the block migration which doesn't exist anymore since it was removed in QEMU v9.1. + +Identifiers +----------- + +Possibility to intersect qdev ids/paths, block node names, and block +export names namespaces is deprecated. In future that would be +abandoned and all block exports, block nodes and devices will have +unique names. Now, reusing the name for another type of object (for +example, creating block-node with node-name equal to existing qdev +id) produce a warning. diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index ed89999f0f..5014324241 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -318,4 +318,6 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host, size_t size); void bdrv_cancel_in_flight(BlockDriverState *bs); +bool check_existing_parent_id(const char *id, Error **errp); + #endif /* BLOCK_GLOBAL_STATE_H */ diff --git a/stubs/check-existing-parent-id.c b/stubs/check-existing-parent-id.c new file mode 100644 index 0000000000..ef5ea3f26d --- /dev/null +++ b/stubs/check-existing-parent-id.c @@ -0,0 +1,6 @@ +#include "qemu/osdep.h" +#include "block/block-global-state.h" + +bool check_existing_parent_id(const char *id, Error **errp) { + return true; +} diff --git a/stubs/meson.build b/stubs/meson.build index 30536ec8fa..3bd8649bd1 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -16,6 +16,7 @@ if have_block stub_ss.add(files('blk-commit-all.c')) stub_ss.add(files('blk-exp-close-all.c')) stub_ss.add(files('blk-by-qdev-id.c')) + stub_ss.add(files('check-existing-parent-id.c')) stub_ss.add(files('blk-exp-find-by-blk.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index be18902bb2..ca14135191 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -593,6 +593,7 @@ static BusState *qbus_find(const char *path, Error **errp) const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) { ObjectProperty *prop; + Error *local_err = NULL; assert(!dev->id && !dev->realized); @@ -601,6 +602,18 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) * has no parent */ if (id) { + g_autofree char *fullpath = g_strdup_printf("/peripheral/%s", id); + + if (!check_existing_parent_id(id, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + + if (!check_existing_parent_id(fullpath, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + prop = object_property_try_add_child(qdev_get_peripheral(), id, OBJECT(dev), NULL); if (prop) { @@ -613,6 +626,13 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) } else { static int anon_count; gchar *name = g_strdup_printf("device[%d]", anon_count++); + g_autofree char *fullpath = g_strdup_printf("/peripheral-anon/%s", id); + + if (!check_existing_parent_id(fullpath, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + prop = object_property_add_child(qdev_get_peripheral_anon(), name, OBJECT(dev)); g_free(name); -- 2.52.0
participants (1)
-
Vladimir Sementsov-Ogievskiy