Re: [PATCH 8/8] qapi: add blockdev-replace command

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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?

[...]