Igor Mammedov <imamm...@redhat.com> writes:
> On Tue, 23 May 2023 14:31:30 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Igor Mammedov <imamm...@redhat.com> writes:
>>
>> > QEMU aborts when default RAM backend should be used (i.e. no
>> > explicit '-machine memory-backend=' specified) but user
>> > has created an object which 'id' equals to default RAM backend
>> > name used by board.
>> >
>> > $QEMU -machine pc \
>> > -object memory-backend-ram,id=pc.ram,size=4294967296
>> >
>> > Actual results:
>> > QEMU 7.2.0 monitor - type 'help' for more information
>> > (qemu) Unexpected error in object_property_try_add() at
>> > ../qom/object.c:1239:
>> > qemu-kvm: attempt to add duplicate property 'pc.ram' to object (type
>> > 'container')
>> > Aborted (core dumped)
>> >
>> > Instead of abort, check for the conflicting 'id' and exit with
>> > an error, suggesting how to remedy the issue.
>>
>> This is an instance of an (unfortunately common) anti-pattern.
>>
>> The point of an ID is to *identify*. To do that, IDs of the same kind
>> must be unique. "Of the same kind" because we let different kinds of
>> objects have the same ID[*].
>>
>> IDs are arbitrary strings. The user may pick any ID, as long as it's
>> unique. Unique not only among the user's IDs, but the system's, too.
>>
>> Every time we add code that picks an ID, we break backward
>> compatibility: user configurations that use this ID no longer work.
>> Thus, system-picked IDs are part of the external interface.
>
> in this case, IDs are there to keep backward compatibility
> (so migration won't fail) and it affects only default (legacy**)
> path where user doesn't provide memory-backend explicitly
> (which could be named anything that doesn't collide with other objects)
>
>> We don't treat them as such. They are pretty much undocumented, and
>> when we add new ones, we break the external interface silently.
>
> this ID in particular is introspect-able (a part of qmp_query_machines output)
> to help mgmt pick backward compatible ID when switching to explicit
> RAM backend CLI (current libvirt behaviour).
>
>> How exactly things go wrong on a clash is detail from an interface
>> design point of view. This patch changes one instance from "crash" to
>> "fatal error". No objections, just pointing out we're playing whack a
>> mole there.
>>
>> The fundamental mistake we made was not reserving IDs for the system's
>> own use.
>>
>> The excuse I heard back then was that IDs are for the user, and the
>> system isn't supposed to pick any. Well, it does.
>>
>> To stop creating more moles, we need to reserve IDs for the system's
>> use, and let the system pick only reserved IDs going forward.
>>
>> There would be two kinds of reserved IDs: 1. an easily documented,
>> easily checked ID pattern, e.g. "starts with <prefix>", to be used by
>> the system going forward, and 2. the messy zoo of system IDs we have
>> accumulated so far.
>>
>> Thoughts?
>
> I'd vote for #1 only, however that isn't an option
> as renaming existing internal IDs will for sure break backward compat.
Yes. Reducing the zoo by dropping and/or renaming IDs would be slow and
painful at best.
> So perhaps a mix of #1 (for all new internal IDs) and #2 for
> legacy ones, with some centralized place to keep track of them.
Yes, this is what I had in mind.
Action items:
1. Reserve ID name space for the system's use
1.a. Document
1.b. Stop letting users use reserved IDs
Either deprecate & warn for a while, then reject, or reject right
away.
2. Grandfather existing system-picked IDs
The ones we care to track down we can document as reserved.
The others we'll have to hand-wave.
Makes sense?
>> [...]
>>
>>
>> [*] Questionable idea if you ask me, but tangential to the point I'm
>> trying to make in this memo.
>>
>
> [**] If it were up to me, I'd drop implicit RAM backend creation
> and require explicit backend being provided on CLI by user
> instead of making thing up for the sake of convenience.
> (If there is a support in favor of this, I'll gladly post a patch)
I lack the expertise to advise on this.
