Re: [PATCH 1/3] scripts/qapi: enum with conditional first item must be optional

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il sab 25 lug 2026, 08:15 Markus Armbruster <armbru@redhat.com> ha scritto:
>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>> > Prevent an all-zero struct from having different meanings with
>> > different configurations or builds of QEMU.
>>
>> This is a bit terse.  What *exactly* is broken?  Spelling this out
>> matters, because it can expose holes in the argument, if any.
>
>
> Nothing is broken; it's just something that can have unexpected
> consequences...
>
>> The numeric encoding of enum values is irrelevant except for zero,
>> because we actually use numeric zero via zero initialization.  If the
>> enum's first value is conditional, the meaning of zero depends on build
>> configuration.  We don't want such a default at the external interface.
>> It could conceivably lead to purely internal bugs, too.
>>
>
> ... like these.

I count 168 enum types including tests, 12 have conditional members, and
none of them have a first member that is conditional.  So this is merely
a latent issue.  Doesn't mean we shouldn't get rid of it, of course.

>> However, I'm not sure your solution fixes this problem completely.
>>
>> Consider type Arg with a mandatory member @mand and an optional enum
>> member @opt, both of enum type ENUM_TYPE.
>>
>> QMP input gets converted to native C like this:
>>
>>     if (!visit_type_Arg(v, NULL, &arg, errp)) {
>>         return;
>>     }
>>
>> If @opt is absent, then arg.has_opt and arg.opt are both zero.
>>
>> Code providing an explicit default then is still fine:
>>
>>     if (!arg.has_opt) {
>>         arg.opt = ENUM_TYPE_MUMBLE;
>>     }
>>
>> But we often use arg.opt without checking arg.has_opt for brevity.  This
>> is an implicit default to zero, whatever zero may mean.
>>
>> If the the optional enum's first member is conditional, this default
>> depends on build configuration.
>>
>> Stupidest solution that could possibly work: an enum's first member
>> cannot be conditional.
>>
>
> That would prevent an enum that is entirely compiled out, which seems like
> a plausibly desirable feature.
>
> Honestly I think this is a C problem, not a QAPI problem. Skipping has_xx
> for bools that default to false is already borderline; doing it for enums
> is well into "you shouldn't do it" territory.

Let's review how we represent optional members in C.

* Values of absent members are zero-initialized.

* There may be a bool has_member that is false when the member is
  absent, true when present.

* We commonly supply default values like this:

      member = obj.has_member ? obj.member : default_value;

  We then use member without checking .has_member again.

* Anything that maps to pointers: since null is not a valid value, we
  use it to represent "absent".  There is no obj.has_member then; we use
  !!obj.member instead.

* Except for arrays, which we represent as singly linked list[*], where
  null means empty: we need has_member.

  Absent array usually defaults to empty.  There's no need to make this
  explicit like

      member = obj.has_member ? obj.member : NULL;

  because obj.member is already null.  So we don't for brevity's sake,
  we just use obj.member.

  Since obj.member has the correct default value, there is no need to
  check .has_member, so we don't.

  In particular, we use code like

      for (elt = obj.member; elt; elt = elt->next) ...

   instead of guarding it with if (obj.has_member) ...

* Anything that doesn't map to pointers: we need has_member.

  When the default is zero, which is fairly common, there's again no
  need to explicitly supply it.  We just use obj.member, without
  cluttering the code with .has_member conditionals.

In my opinion, this (non-)usage of .has_member is just *fine* as long as
the meaning of zero is well-defined at the QAPI level.

For bool and and numbers, the meaning isn't just well-defined, it's also
completely obvious.

It's well-defined for enums as long as the enum's first member is
unconditional.  The meaning of zero is admittedly less obvious there.

We could certainly change the existing code to only read obj.member
where guarded by if (obj.has_member).  Inhowfar we'd then succeed at
keeping the shorter (and in my opinion more readable) forms from
creeping back is less certain.  But what's the benefit?  Enabling "a
plausibly desirable feature" we haven't found a use for is one, but I
don't think it can justify the change.  Are there other benefits I don't
see?  For Rust, maybe?

>                                               Whereas "unless you have
> mandatory pointer fields, g_malloc0(...) returns a valid *and portable*
> QAPI struct" is in my opinion supporting a desirable idiom.

We do rely on zero-initialization to do the right thing.

> Rust would spell it "Default::default()"; the language can help rejecting
> it if you have mandatory string fields (it would recursively default boxed
> structs, unlike C) but it cannot do anything about portability; this patch
> closes the gap completely for Rust, and does what it can for C.

I wouldn't call it a portability problem.  The actual problem is that
build-time configuration can have unwanted effects at least in theory.
Since different host platforms can require different configuration,
porting can trigger the problem.  I.e. it's a special case.  But I'm
getting close to splitting hairs, and should stop :)

I'm of course willing to evolve the schema language to help Rust, or to
eliminate ways for us to screw up.

You propose to

* Forbid empty enums [PATCH 2].  They are allowed simply because I never
  found a compelling reason to forbid them.  Yes, they're useless, but
  users can figure that out without the QAPI generator insisting.

  Does forbidding them help Rust?

  You still allow enums whose members are all conditional.  Build
  configuration could make these empty, but there are conceivable uses.

* Require members of enum type to be optional when the enum's first
  member is conditional [PATCH 1].

  Does this help Rust?

I believe these rules are mildly bothersome to explain.  Which you
sidestepped by not documenting them in docs/devel/qapi-code-gen.rst :)

PATCH 1's commit message claims it prevents "an all-zero struct from
having different meanings with different configurations or builds of
QEMU".  This is misleading without further qualifications: it doesn't
actually prevent it *with the existing coding conventions*.  The commit
message would be easy enough to fix, of course.

To actually prevent it reliably, we'd have to attack the root of the
problem, namely the meaning of enum types' zero value.  The obvious way
to do that is to require enums to have an unconditional first member.
This is also simpler to document, I think.

Drawback: it removes the ability to define an enum whose members are all
conditional, usable with optional object members.  Does this matter
enough to complicate things?

> Paolo
>
>> Thoughts?
>>
>> > This needs some changes to doc-good.json, which used unwittingly
>> > such an enum.
>> >
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

[...]

[*] A questionable choice, but here we are.