Il sab 25 lug 2026, 08:15 Markus Armbruster <[email protected]> ha scritto:
> Paolo Bonzini <[email protected]> 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. 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. Whereas "unless you have mandatory pointer fields, g_malloc0(...) returns a valid *and portable* QAPI struct" is in my opinion supporting a desirable idiom. 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. Paolo Thoughts? > > This needs some changes to doc-good.json, which used unwittingly > > such an enum. > > > > Signed-off-by: Paolo Bonzini <[email protected]> > > --- > > scripts/qapi/schema.py | 8 ++++++++ > > tests/qapi-schema/doc-good.json | 16 ++++++++-------- > > tests/qapi-schema/doc-good.out | 12 ++++++------ > > tests/qapi-schema/doc-good.txt | 8 ++++---- > > tests/qapi-schema/enum-if-first-required.err | 2 ++ > > tests/qapi-schema/enum-if-first-required.json | 6 ++++++ > > tests/qapi-schema/enum-if-first-required.out | 0 > > tests/qapi-schema/meson.build | 1 + > > 8 files changed, 35 insertions(+), 18 deletions(-) > > create mode 100644 tests/qapi-schema/enum-if-first-required.err > > create mode 100644 tests/qapi-schema/enum-if-first-required.json > > create mode 100644 tests/qapi-schema/enum-if-first-required.out > > Thanks for the negative test. > > docs/devel/qapi-code-gen.rst section "Enumeration types" could perhaps > use an update. > > It's less than clear even before the patch: > > The generated C enumeration constants have values 0, 1, ..., N-1 (in > QAPI schema order), where N is the number of values. There is an > additional enumeration constant PREFIX__MAX with value N. > > Do not use string or an integer type when an enumeration type can do > the job satisfactorily. > > The optional 'if' member specifies a conditional. See `Configuring the > schema`_ below for more on this. > > The first paragraph can lead readers to assume we generate something > like > > typedef enum Example { > EXAMPLE_FOO = 0, > #if defined(COND) > EXAMPLE_BAR = 1, > #endif > EXAMPLE__MAX = 2, > } Example; > > We don't, because we'd have to deal with a "hole" in the value range > when COND is not defined. > > Instead, we do > > typedef enum Example { > EXAMPLE_FOO, > #if defined(COND) > EXAMPLE_BAR, > #endif > EXAMPLE__MAX, > } Example; > >
