Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes:
> Add command that can add and remove filters.
>
> Key points of functionality:
>
> What the command does is simply replace some BdrvChild.bs by some other
> nodes. The tricky thing is selecting there BdrvChild objects.
> To be able to select any kind of BdrvChild we use a generic parent_id,
> which may be a node-name, or qdev id or block export id. In future we
> may support block jobs.
>
> Any kind of ambiguity leads to error. If we have both device named
> device0 and block export named device0 and they both point to same BDS,
> user can't replace root child of one of these parents. So, to be able
> to do replacements, user should avoid duplicating names in different
> parent namespaces.
>
> So, command allows to replace any single child in the graph.
>
> On the other hand we want to realize a kind of bdrv_replace_node(),
> which works well when we want to replace all parents of some node. For
> this kind of task @parents-mode argument implemented.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> qapi/block-core.json | 78 +++++++++++++++++++++++++++++++++++++++++
> block.c | 82 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 160 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 675d8265eb..8059b96341 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5433,3 +5433,81 @@
> { 'command': 'blockdev-snapshot-delete-internal-sync',
> 'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
> 'returns': 'SnapshotInfo' }
> +
> +##
> +# @BlockdevReplaceParentsMode:
> +#
> +# Alternative (to directly set @parent) way to chose parents in
> +# @blockdev-replace
> +#
> +# @exactly-one: Exactly one parent should match a condition, otherwise
> +# @blockdev-replace fails.
> +#
> +# @all: All matching parents are taken into account. If replacing lead
> +# to loops in block graph, @blockdev-replace fails.
> +#
> +# @auto: Same as @all, but automatically skip replacing parents if it
> +# leads to loops in block graph.
> +#
> +# Since: 6.2
> +##
> +{ 'enum': 'BlockdevReplaceParentsMode',
> + 'data': ['exactly-one', 'all', 'auto'] }
> +
> +##
> +# @BlockdevReplace:
> +#
> +# Declaration of one replacement.
Replacement of what? A node in the block graph?
> +#
> +# @parent: id of parent. It may be qdev or block export or simple
> +# node-name.
It may also be a QOM path, because find_device_state() interprets
arguments starting with '/' as QOM paths.
When is a node name "simple"?
Suggest: It may be a qdev ID, a QOM path, a block export ID, or a node
name.
The trouble is of course that we're merging three separate name spaces.
Aside: a single name space for IDs would be so much saner, but we
screwed that up long ago.
> If id is ambiguous (for example node-name of
> +# some BDS equals to block export name), blockdev-replace
> +# fails.
Is there a way out of this situation, or are is replacement simply
impossible then?
> If not specified, blockdev-replace goes through
> +# @parents-mode scenario, see below. Note, that @parent and
> +# @parents-mode can't be specified simultaneously.
What if neither is specified? Hmm, @parents-mode has a default, so
that's what we get.
> +# If @parent is specified, only one edge is selected. If
> +# several edges match the condition, blockdev-replace fails.
> +#
> +# @edge: name of the child. If omitted, any child name matches.
> +#
> +# @child: node-name of the child. If omitted, any child matches.
> +# Must be present if @parent is not specified.
Is @child useful when @parent is present?
What's the difference between "name of the child" and "node name of the
child"?
> +#
> +# @parents-mode: declares how to select edge (or edges) when @parent
> +# is omitted. Default is 'one'.
'exactly-one'
Minor combinatorial explosion. There are four optional arguments, one
of them an enum, and only some combination of argument presence and enum
value are valid. For a serious review, I'd have to make a table of
combinations, then think through every valid row.
Have you considered making this type a union? Can turn some of your
semantic constraints into syntactical ones. Say you turn
BlockdevReplaceParentsMode into a tag enum by adding value 'by-id'.
Then branch 'by-id' has member @parent, and the others don't.
> +#
> +# Since: 6.2
> +#
> +# Examples:
> +#
> +# 1. Change root node of some device.
> +#
> +# Note, that @edge name is omitted, as
Scratch "name".
Odd line break.
> +# devices always has only one child. As well, no need in specifying
> +# old @child.
"the old @child".
> +#
> +# -> { "parent": "device0", "new-child": "some-node-name" }
> +#
> +# 2. Insert copy-before-write filter.
> +#
> +# Assume, after blockdev-add we have block-node 'source', with several
> +# writing parents and one copy-before-write 'filter' parent. And we want
> +# to actually insert the filter. We do:
> +#
> +# -> { "child": "source", "parent-mode": "auto", "new-child": "filter" }
> +#
> +# All parents of source would be switched to 'filter' node, except for
> +# 'filter' node itself (otherwise, it will make a loop in block-graph).
Good examples. I think we need more, to give us an idea on the use
cases for the combinatorial explosion. I need to know them to be able
to review the interface.
> +##
> +{ 'struct': 'BlockdevReplace',
> + 'data': { '*parent': 'str', '*edge': 'str', '*child': 'str',
> + '*parents-mode': 'BlockdevReplaceParentsMode',
> + 'new-child': 'str' } }
> +
> +##
> +# @blockdev-replace:
> +#
> +# Do one or several replacements transactionally.
> +##
> +{ 'command': 'blockdev-replace',
> + 'data': { 'replacements': ['BlockdevReplace'] } }
Ignorant question: integration with transaction.json makes no sense?
[...]
