Vladimir Sementsov-Ogievskiy <[email protected]> writes:
> On 14.02.26 11:04, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <[email protected]> writes:
>>
>>> On 04.02.26 15:26, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <[email protected]> writes:
>>>>
>>>>> 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 <[email protected]>
[...]
>>>> Let's see whether I understand...
>>>>
>>>> @parent determines which of the three cases mentioned in the commit
>>>> message it ids:
>>>>
>>>> * If @parent is a QOM path, case 1.
>>>>
>>>> * If @parent is a block export name (@id in BlockExportOptions and
>>>> BlockExportInfo), case 2.
>>>>
>>>> * If @parent is a block node name (@node-name in BlockdevOptions and
>>>> BlockDeviceInfo), case 3.
>>>>
>>>> Correct?
>>>>
>>>> Problem: this is ambiguous. A @parent "foo" could in fact be any of the
>>>> three cases.
>>>
>>> Yes. And we return an error in case of any ambiguity.
>>
>> So, in case of ambiguity, the command does not work.
>>
>> On the one hand, this is the user's doing: reusing the same ID for
>> multiple things has always been a bad idea.
>>
>> On the other hand, we're now turning a bad idea that may cause confusion
>> into an bad idea that may break things. Feels iffy.
>>
>> For QOM paths, we have a workaround: avoid partial QOM paths. Feels
>> okay. Partial QOM paths are rather obscure, and best avoided entirely.
>>
>> The only workaround for block export vs. block node ambiguity is to
>> avoid ID reuse. By the time a user sees the command fail, the IDs are
>> what they are, and there's is no workaround.
>>
>> Do you need blockdev-replace to work for both *now*?
>>
>> At the very least, we need to document the trap and point to the
>> workaround.
>>
>> Alternatively, come up with an interface that avoids the issue. See
>> below.
>>
>>>> 3. If a block node named "foo" exists, it's case 3.
>>>>
>>>> 2. If a block export named "foo" exists, it's also case 2.
>>>>
>>>> 1. If exactly one QOM object named "foo" exists, it's also case 1. Why?
>>>> "foo" is a syntactically valid partial QOM path. Partial QOM paths
>>>> are a convenience feature that is virtually unknown (and possibly
>>>> ill-advised): you can omit leading path components as long as there's
>>>> no ambiguity.
>>>>
>>>> Peeking ahead in the series... PATCH 8 appears to deprecate the
>>>> ambiguity between case 2. and 3.
>>>>
>>>> I think we need to do better.
>>
>> See review of PATCH 8 for doing better on deprecation.
>>
>>>> More questions...
>>>>
>>>> In case 1 and 2, @child "should be root". What happens when it's
>>>> something else?
>>>
>>> Error returned. s/should/must/ ?
>>
>> Yes, please.
>>
>>>> In case 3, @child "should be any if its children names". I figure the
>>>> command fails when it isn't.
>>>
>>> And here.
>>
>> Likewise.
>>
>>>>> +{ 'command': 'blockdev-replace',
>>>>> + 'data': { 'parent': 'str', 'child': 'str', 'new-child': 'str' } }
>>>>
>>>> [...]
>>
>> This interface sort of overloads @parent and @child, and overloading the
>> former creates ambiguity that can make the command unusable.
>
> Is it a real problem if we do deprecate the thing, that leads to this
> ambiguity?
>
> If it is, we may mark the command "unstable" during the deprecation period.
By itself, the proposed command is a trap for the unwary.
Deprecating the usage that enables the trap makes the trap temporary.
Warning on (deprecated) usage that enables the trap helps users avoid
it. Human users, mostly.
Marking the command "unstable" should ensure management application
avoid the command, and thus the trap.
All of the above together feels acceptable to me. Can we do better?
Maybe, and that's discussed below.
> Or even postpone its addition up to the end of this period.
No trap, no problem :)
>> @parent's set of valid values is the union of QOM path, block node name,
>> block export ID with values occuring in more than one of them dropped.
>>
>> @child's set of valid values depends on @parent: child name when it's a
>> QOM path, else "root".
>>
>> The obvious non-overloaded interface is a union of three branches: QOM,
>> block node, block export. The QOM branch has variant members @qom-path
>> and @child. The block node branch has variant member @node-name. The
>> block export branch has variant member @id.
>>
>> This is a bit more verbose: you have to specify the union tag[*].
>>
>> If we ever need to replace block nodes associated with additional
>> things, we simply more branches. These branches can have arbitrary
>> members. In your interface, we'd have to make do with @child, or maybe
>> add optional arguments.
>>
>> Thoughts? Mind, this isn't a demand! I'm exploring the design space.
>
> Union-based solution was v9:
> https://lore.kernel.org/qemu-devel/[email protected]/
>
> And there was a discussion, that we may try a way without a union. And that's
> this v10.
I see Kevin suggested to explore this approach. I certainly respect his
judgement.
> Hmm, but that had different field names for parents: qdev-id, export-id and
> node-name.
>
> What you suggest is to keep one filed for parent - "parent", but also add a
> "parent-type"
> field. This way, we may deprecate and remove "parent-type" in future. Or, we
> may
> add it as deprecated now (together with deprecating non-unique IDs)
We can't deprecate the union tag right away: it is *required*.
>> [*] We could add a "may omit union tag when the tag value can be derived
>> from present variant members" convenience feature to QAPI, but I'm not
>> sure it's worth the complexity.
>>
>
> It also may be not a union, but a simple structure with optional fields:
>
> {
> parent: str, required, qom-path, or node-name, or export name
> child: str, optional, but required when parent is node-name, and must be
> 'root'
> if present for other parent types
> parent-type: str, optional, "qom-path" | "block-node" | "block-export",
> helps to
> overcome parent ambiguity, deprecated
> new-child: str, required node-name
> }
PRO union: which members are valid together is syntactically obvious.
With a bunch of optional members, that information is in the member
descriptions.
CON union: need to specify the union tag. The verbosity is a non-issue
for management applications, and mildly bothersome for humans.
How important is keeping things terse for humans here?
