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:

> 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.  Let me
try.

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.

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;
    }

For members present in the input, there is no zero initialization
problem.

@mand is always present, so arg.mand is whatever the user said.

If @opt is present, arg.has_opt is true and arg.opt is whatever the user
said.

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.

Thoughts?

> This needs some changes to doc-good.json, which used unwittingly
> such an enum.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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;