Re: [PATCH v9 4/7] qapi: add blockdev-replace command

[Kevin Wolf <kwolf@redhat.com>]

Am 04.10.2024 um 19:01 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 02.10.24 17:41, Vladimir Sementsov-Ogievskiy wrote:
> > On 26.06.24 14:53, Vladimir Sementsov-Ogievskiy wrote:
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index df5e07debd..0a6f08a6e0 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -6148,3 +6148,91 @@
> > >   ##
> > >   { 'struct': 'DummyBlockCoreForceArrays',
> > >     'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
> > > +
> > > +##
> > > +# @BlockParentType:
> > > +#
> > > +# @qdev: block device, such as created by device_add, and denoted by
> > > +#     qdev-id
> > > +#
> > > +# @driver: block driver node, such as created by blockdev-add, and
> > > +#     denoted by node-name
> > > +#
> > > +# @export: block export, such created by block-export-add, and
> > > +#     denoted by export-id
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'enum': 'BlockParentType',
> > > +  'data': ['qdev', 'driver', 'export'] }
> > > +
> > > +##
> > > +# @BdrvChildRefQdev:
> > > +#
> > > +# @qdev-id: the device's ID or QOM path
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'struct': 'BdrvChildRefQdev',
> > > +  'data': { 'qdev-id': 'str' } }
> > > +
> > > +##
> > > +# @BdrvChildRefExport:
> > > +#
> > > +# @export-id: block export identifier
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'struct': 'BdrvChildRefExport',
> > > +  'data': { 'export-id': 'str' } }
> > > +
> > > +##
> > > +# @BdrvChildRefDriver:
> > > +#
> > > +# @node-name: the node name of the parent block node
> > > +#
> > > +# @child: name of the child to be replaced, like "file" or "backing"
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'struct': 'BdrvChildRefDriver',
> > > +  'data': { 'node-name': 'str', 'child': 'str' } }
> > > +
> > > +##
> > > +# @BlockdevReplace:
> > > +#
> > > +# @parent-type: type of the parent, which child is to be replaced
> > > +#
> > > +# @new-child: new child for replacement
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'union': 'BlockdevReplace',
> > > +  'base': {
> > > +      'parent-type': 'BlockParentType',
> > > +      'new-child': 'str'
> > > +  },
> > > +  'discriminator': 'parent-type',
> > > +  'data': {
> > > +      'qdev': 'BdrvChildRefQdev',
> > > +      'export': 'BdrvChildRefExport',
> > > +      'driver': 'BdrvChildRefDriver'
> > > +  } }
> > > +
> > > +##
> > > +# @blockdev-replace:
> > > +#
> > > +# Replace a block-node associated with device (selected by
> > > +# @qdev-id) or with block-export (selected by @export-id) or
> > > +# any child of block-node (selected by @node-name and @child)
> > > +# with @new-child block-node.
> > > +#
> > > +# Features:
> > > +#
> > > +# @unstable: This command is experimental.
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'command': 'blockdev-replace', 'boxed': true,
> > > +  'features': [ 'unstable' ],
> > > +  'data': 'BlockdevReplace' }
> > 
> > 
> > Looking back at this all, I now have another idea: instead of trying
> > to unite different types of parents, maybe just publish concept of
> > BdrvChild to QAPI? So that it will have unique id. Like for
> > block-nodes, it could be auto-generated or specified by user.
> > 
> > Then we'll add parameters for commands:
> > 
> > device-add
> >     root-child-slot-id: ID
> > 
> > block-export-add
> >     block-child-slot-id: ID
> > 
> > and for block-drivers we already have BlockdevRef structure, it only
> > lacks an id.
> > 
> > { 'alternate': 'BlockdevRef',
> >    'data': { 'definition': 'BlockdevOptions',
> >              'reference': 'str' } }
> > 
> > hmm.. Could it be as simple as
> > 
> > 
> > { 'alternate': 'BlockdevRef',
> >    'base': { '*child-slot-id': 'str' },
> >    'data': { 'definition': 'BlockdevOptions',
> >              'reference': 'str' } }
> > 
> > ?
> 
> Oops that was obviously impossible idea :) Then, I think the only way
> is to introduce virtual "reference" BlockdevDriver, with only one
> parameter { 'reference': 'str' }, this way user will be able to
> specify
> 
> file: {driver: reference, reference: NODE_NAME, child-slot-id: NEW_ID}

I don't think adding such a hack would make the interface any nicer
compared to what you have now with the union.

If we want to get rid of the union, I think the best course of action
would unifying the namespaces (so that nodes, exports and devices can't
share the same ID) and then we could just accept a universal 'id' along
with 'child'.

This would mean having 'child' even for devices, which feels
unnecessary at least in the common case, but it would at the same time
resolve Markus' concern if there could be any devices with multiple
BlockBackends.

I can only think of isa-fdc that used to be such a device, but that was
just incorrect modelling on the qdev level. Not sure if there are actual
legitimate use cases for having multiple BlockBackends.

Anyway, for the moment, I would suggest going ahead with the union.

Kevin