Re: The madness of ad hoc special IDs

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Tue, 23 May 2023 14:31:30 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Igor Mammedov <imammedo@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.