[PATCH resend 0/3] scripts/qapi: non-Rust-specific preparatory patches

[PATCH resend 0/3] scripts/qapi: non-Rust-specific preparatory patches

[Paolo Bonzini]

This series include a few small QAPI patches extracted out of the Rust
series.

The first two tighten the language in ways that are necessary for Rust (the
structs would not be representable) but probably a good idea in general.

The last one moves C-specific knowledge out of functions with a generic
name.

Paolo

Paolo Bonzini (3):
  scripts/qapi: enum with conditional first item must be optional
  scripts/qapi: reject empty enums
  scripts/qapi: pull c_name and lstrip from camel_to_upper to caller

 scripts/qapi/common.py                        |  4 ++--
 scripts/qapi/schema.py                        | 12 ++++++++++++
 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-empty.err              |  2 ++
 tests/qapi-schema/enum-empty.json             |  2 ++
 tests/qapi-schema/enum-empty.out              |  0
 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                 |  2 ++
 tests/qapi-schema/qapi-schema-test.json       |  3 ---
 tests/qapi-schema/qapi-schema-test.out        |  1 -
 tests/qapi-schema/union-empty-if.err          |  2 ++
 tests/qapi-schema/union-empty-if.json         |  6 ++++++
 tests/qapi-schema/union-empty-if.out          |  0
 tests/qapi-schema/union-empty.err             |  4 ++--
 18 files changed, 56 insertions(+), 26 deletions(-)
 create mode 100644 tests/qapi-schema/enum-empty.err
 create mode 100644 tests/qapi-schema/enum-empty.json
 create mode 100644 tests/qapi-schema/enum-empty.out
 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
 create mode 100644 tests/qapi-schema/union-empty-if.err
 create mode 100644 tests/qapi-schema/union-empty-if.json
 create mode 100644 tests/qapi-schema/union-empty-if.out

-- 
2.55.0

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

[Paolo Bonzini]

Prevent an all-zero struct from having different meanings with
different configurations or builds of QEMU.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8d88b40de2e..71296b15de2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -963,6 +963,14 @@ def check(self, schema: QAPISchema) -> None:
         assert self.defined_in
         self.type = schema.resolve_type(self._type_name, self.info,
                                         self.describe)
+        if (not self.optional
+                and isinstance(self.type, QAPISchemaEnumType)
+                and self.type.members[0].ifcond.is_present()):
+            raise QAPISemError(
+                self.info,
+                "enum type '%s' of %s has a conditional first value"
+                " and must be optional"
+                % (self.type.name, self.describe(self.info)))
         seen: Dict[str, QAPISchemaMember] = {}
         for f in self.features:
             f.check_clash(self.info, seen)
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index fac13425b72..76521ffe9e6 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -73,12 +73,12 @@
 # @enum-feat: Also _one_ {and only}
 # @enum-member-feat: a member feature
 #
-# @two is undocumented
+# @zero is undocumented
 ##
 { 'enum': 'Enum',
-  'data': [ { 'name': 'one', 'if': 'IFONE',
-              'features': [ 'enum-member-feat' ] },
-            'two' ],
+  'data': [ 'zero',
+            { 'name': 'one', 'if': 'IFONE',
+               'features': [ 'enum-member-feat' ] } ],
   'features': [ 'enum-feat' ],
   'if': 'IFCOND' }
 
@@ -112,10 +112,10 @@
                       'if': 'IFSTR' } } }
 
 ##
-# @Variant2:
+# @Variant0:
 #
 ##
-{ 'struct': 'Variant2', 'data': {} }
+{ 'struct': 'Variant0', 'data': {} }
 
 ##
 # @Object:
@@ -128,8 +128,8 @@
   'base': 'Base',
   'discriminator': 'base1',
   'data': { 'one': 'Variant1',
-            'two': { 'type': 'Variant2',
-                     'if': { 'any': ['IFONE', 'IFTWO'] } } } }
+            'zero': { 'type': 'Variant0',
+                      'if': { 'any': ['IFONE', 'IFTWO'] } } } }
 
 ##
 # @Alternate:
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 371dd25ffc7..2782f6b57d9 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -10,10 +10,10 @@ enum QType
     member qbool
 module doc-good.json
 enum Enum
+    member zero
     member one
         if IFONE
         feature enum-member-feat
-    member two
     if IFCOND
     feature enum-feat
 object Base
@@ -24,12 +24,12 @@ object Variant1
         if IFSTR
         feature member-feat
     feature variant1-feat
-object Variant2
+object Variant0
 object Object
     base Base
     tag base1
     case one: Variant1
-    case two: Variant2
+    case zero: Variant0
         if {'any': ['IFONE', 'IFTWO']}
     feature union-feat1
 alternate Alternate
@@ -110,14 +110,14 @@ doc symbol=Enum
 
     Member=one
 The _one_ {and only}, description on the same line
-    Member=two
+    Member=zero
 
     Feature=enum-feat
 Also _one_ {and only}
     Feature=enum-member-feat
 a member feature
     Plain
-@two is undocumented
+@zero is undocumented
 doc symbol=Base
     Intro
 
@@ -139,7 +139,7 @@ Another paragraph
 a feature
     Feature=member-feat
 a member feature
-doc symbol=Variant2
+doc symbol=Variant0
     Intro
 
 doc symbol=Object
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index 74b73681d32..922a61dcf23 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -43,14 +43,14 @@ Enum Enum
    Values:
       * **one** -- The _one_ {and only}, description on the same line
 
-      * **two** -- Not documented
+      * **zero** -- Not documented
 
    Features:
       * **enum-feat** -- Also _one_ {and only}
 
       * **enum-member-feat** -- a member feature
 
-   "two" is undocumented
+   "zero" is undocumented
 
 Object Base
     *Availability*: "IFALL1 and IFALL2"
@@ -75,7 +75,7 @@ Object Variant1
 
       * **member-feat** -- a member feature
 
-Object Variant2
+Object Variant0
 
 Object Object
 
@@ -84,7 +84,7 @@ Object Object
 
       * When "base1" is "one": The members of "Variant1".
 
-      * When "base1" is "two": The members of "Variant2".
+      * When "base1" is "zero": The members of "Variant0".
 
    Features:
       * **union-feat1** -- a feature
diff --git a/tests/qapi-schema/enum-if-first-required.err b/tests/qapi-schema/enum-if-first-required.err
new file mode 100644
index 00000000000..6d8bdcf2507
--- /dev/null
+++ b/tests/qapi-schema/enum-if-first-required.err
@@ -0,0 +1,2 @@
+enum-if-first-required.json: In struct 'TestStruct':
+enum-if-first-required.json:5: enum type 'TestEnum' of member 'field' has a conditional first value and must be optional
diff --git a/tests/qapi-schema/enum-if-first-required.json b/tests/qapi-schema/enum-if-first-required.json
new file mode 100644
index 00000000000..1769b5fdef9
--- /dev/null
+++ b/tests/qapi-schema/enum-if-first-required.json
@@ -0,0 +1,6 @@
+# Enum with conditional first value cannot be used in required fields
+{ 'enum': 'TestEnum',
+  'data': [ { 'name': 'member1', 'if': 'CONFIG_FOO' },
+            'member2' ] }
+{ 'struct': 'TestStruct',
+  'data': { 'field': 'TestEnum' } }
diff --git a/tests/qapi-schema/enum-if-first-required.out b/tests/qapi-schema/enum-if-first-required.out
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index debff633ac1..3b0c16a8b67 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -97,6 +97,7 @@ schemas = [
   'enum-bad-prefix.json',
   'enum-clash-member.json',
   'enum-dict-member-unknown.json',
+  'enum-if-first-required.json',
   'enum-if-invalid.json',
   'enum-int-member.json',
   'enum-member-case.json',
-- 
2.55.0

[PATCH 2/3] scripts/qapi: reject empty enums

[Paolo Bonzini]

Raise an error if an enum has no members.  Such enums cannot be populated
with a valid value, and therefore they can only be used as optional
members.  But QAPI leaves out optional members from the serialization
protocol, and therefore the existence of the enum is completely pointless.

The same limitation is not extended to an enum whose members are all
compiled out; these can still be useful as optional members.  However they
cannot implicitly block usage of parent structures because, by virtue
of having only conditional members, they are "enums with conditional
first item" and must therefore be optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/qapi/schema.py                  | 4 ++++
 tests/qapi-schema/enum-empty.err        | 2 ++
 tests/qapi-schema/enum-empty.json       | 2 ++
 tests/qapi-schema/enum-empty.out        | 0
 tests/qapi-schema/meson.build           | 1 +
 tests/qapi-schema/qapi-schema-test.json | 3 ---
 tests/qapi-schema/qapi-schema-test.out  | 1 -
 tests/qapi-schema/union-empty-if.err    | 2 ++
 tests/qapi-schema/union-empty-if.json   | 6 ++++++
 tests/qapi-schema/union-empty-if.out    | 0
 tests/qapi-schema/union-empty.err       | 4 ++--
 11 files changed, 19 insertions(+), 6 deletions(-)
 create mode 100644 tests/qapi-schema/enum-empty.err
 create mode 100644 tests/qapi-schema/enum-empty.json
 create mode 100644 tests/qapi-schema/enum-empty.out
 create mode 100644 tests/qapi-schema/union-empty-if.err
 create mode 100644 tests/qapi-schema/union-empty-if.json
 create mode 100644 tests/qapi-schema/union-empty-if.out

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 71296b15de2..a5a11298817 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -428,6 +428,10 @@ def __init__(
 
     def check(self, schema: QAPISchema) -> None:
         super().check(schema)
+        if not self.members:
+            raise QAPISemError(
+                self.info,
+                "enum '%s' must have at least one value" % self.name)
         seen: Dict[str, QAPISchemaMember] = {}
         for m in self.members:
             m.check_clash(self.info, seen)
diff --git a/tests/qapi-schema/enum-empty.err b/tests/qapi-schema/enum-empty.err
new file mode 100644
index 00000000000..6070bf62cbd
--- /dev/null
+++ b/tests/qapi-schema/enum-empty.err
@@ -0,0 +1,2 @@
+enum-empty.json: In enum 'TestEmpty':
+enum-empty.json:2: enum 'TestEmpty' must have at least one value
diff --git a/tests/qapi-schema/enum-empty.json b/tests/qapi-schema/enum-empty.json
new file mode 100644
index 00000000000..3b3dfb2e3d8
--- /dev/null
+++ b/tests/qapi-schema/enum-empty.json
@@ -0,0 +1,2 @@
+# An enum must have at least one value
+{ 'enum': 'TestEmpty', 'data': [] }
diff --git a/tests/qapi-schema/enum-empty.out b/tests/qapi-schema/enum-empty.out
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 3b0c16a8b67..1058b7ecfae 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -196,6 +196,7 @@ schemas = [
   'union-clash-member.json',
   'union-discriminator-bad-name.json',
   'union-empty.json',
+  'union-empty-if.json',
   'union-inline-invalid-dict.json',
   'union-int-branch.json',
   'union-invalid-base.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 8ca977c49d2..195f1c4847b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -23,9 +23,6 @@
   'data': { 'enum1': 'EnumOne',   # Intentional forward reference
             '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
 
-# An empty enum, although unusual, is currently acceptable
-{ 'enum': 'MyEnum', 'data': [ ] }
-
 # Likewise for an empty struct, including an empty base
 { 'struct': 'Empty1', 'data': { } }
 { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 4617eb4e98a..ddd8bf80d66 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -18,7 +18,6 @@ object NestedEnumsOne
     member enum2: EnumOne optional=True
     member enum3: EnumOne optional=False
     member enum4: EnumOne optional=True
-enum MyEnum
 object Empty1
 object Empty2
     base Empty1
diff --git a/tests/qapi-schema/union-empty-if.err b/tests/qapi-schema/union-empty-if.err
new file mode 100644
index 00000000000..2b8691ab2f5
--- /dev/null
+++ b/tests/qapi-schema/union-empty-if.err
@@ -0,0 +1,2 @@
+union-empty-if.json: In struct 'Base':
+union-empty-if.json:5: enum type 'Empty' of member 'type' has a conditional first value and must be optional
diff --git a/tests/qapi-schema/union-empty-if.json b/tests/qapi-schema/union-empty-if.json
new file mode 100644
index 00000000000..36625dc4a89
--- /dev/null
+++ b/tests/qapi-schema/union-empty-if.json
@@ -0,0 +1,6 @@
+# union discriminator enum cannot have only conditional items
+{ 'enum': 'Empty',
+  'data': [ { 'name' : 'bar', 'if': { 'all': ['FOO'] } } ] }
+
+{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
+{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }
diff --git a/tests/qapi-schema/union-empty-if.out b/tests/qapi-schema/union-empty-if.out
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err
index d4284399621..c07dcf32a5a 100644
--- a/tests/qapi-schema/union-empty.err
+++ b/tests/qapi-schema/union-empty.err
@@ -1,2 +1,2 @@
-union-empty.json: In union 'Union':
-union-empty.json:4: union has no branches
+union-empty.json: In enum 'Empty':
+union-empty.json:2: enum 'Empty' must have at least one value
-- 
2.55.0

[PATCH 3/3] scripts/qapi: pull c_name and lstrip from camel_to_upper to caller

[Paolo Bonzini]

Allow using camel_to_upper for other languages too.

In particular, the lstrip() is needed to avoid reserved C identifiers,
for example:

    typedef enum __org_qemu_x_Enum {
        __ORG_QEMU_X_ENUM___ORG_QEMU_X_VALUE,
        __ORG_QEMU_X_ENUM__MAX,
    } __org_qemu_x_Enum;

Insulate Rust from this, since underscores have a different meaning
in Rust (though only for the sake of warnings).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/qapi/common.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index d7c8aa3365c..43a0be5f97f 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -61,7 +61,7 @@ def camel_to_upper(value: str) -> str:
         ret += ch
         upc = ch.isupper()
 
-    return c_name(ret.upper()).lstrip('_')
+    return ret.upper()
 
 
 def c_enum_const(type_name: str,
@@ -75,7 +75,7 @@ def c_enum_const(type_name: str,
     :param prefix: Optional, prefix that overrides the type_name.
     """
     if prefix is None:
-        prefix = camel_to_upper(type_name)
+        prefix = c_name(camel_to_upper(type_name)).lstrip('_')
     return prefix + '_' + c_name(const_name, False).upper()
 
 
-- 
2.55.0

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

[Markus Armbruster]

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;