Re: The madness of ad hoc special IDs (was: [PATCH] machine: do not crash if default RAM backend name has been stollen)

[Igor Mammedov <imammedo@redhat.com>]

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.
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.
 
> [...]
> 
> 
> [*] 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)