diff mbox series

[6/6] qapi: expose all schema features to code

Message ID 20240801175913.669013-7-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi: generalize special features | expand

Commit Message

Daniel P. Berrangé Aug. 1, 2024, 5:59 p.m. UTC 
This removed the QapiFeatures enum and auto-generates an enum which
exposes all features defined by the schema to code.

The 'deprecated' and 'unstable' features still have a little bit of
special handling, being force defined to be the 1st + 2nd features
in the enum, regardless of whether they're used in the schema. This
is because QAPI common code references these features.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/util.h      |   5 --
 meson.build              |   1 +
 scripts/qapi/features.py | 134 +++++++++++++++++++++++++++++++++++++++
 scripts/qapi/main.py     |   2 +
 scripts/qapi/schema.py   |   5 +-
 scripts/qapi/types.py    |   4 +-
 6 files changed, 144 insertions(+), 7 deletions(-)
 create mode 100644 scripts/qapi/features.py

Comments

Markus Armbruster Aug. 2, 2024, 1:50 p.m. UTC | #1 
Doesn't build for me:

    qapi/qapi-init-commands.c: In function ‘qmp_init_marshal’:
    qapi/qapi-init-commands.c:70:84: error: ‘QAPI_FEATURE_ALLOW_WRITE_ONLY_OVERLAY’ undeclared (first use in this function)
       70 |                          qmp_marshal_blockdev_snapshot, QCO_ALLOW_PRECONFIG, 1u << QAPI_FEATURE_ALLOW_WRITE_ONLY_OVERLAY);
          |                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    qapi/qapi-init-commands.c:70:84: note: each undeclared identifier is reported only once for each function it appears in
    qapi/qapi-init-commands.c:464:70: error: ‘QAPI_FEATURE_SAVEVM_MONITOR_NODES’ undeclared (first use in this function)
      464 |                          qmp_marshal_human_monitor_command, 0, 1u << QAPI_FEATURE_SAVEVM_MONITOR_NODES);
          |                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Daniel P. Berrangé Aug. 2, 2024, 3:43 p.m. UTC | #2 
On Fri, Aug 02, 2024 at 03:50:33PM +0200, Markus Armbruster wrote:
> Doesn't build for me:
> 
>     qapi/qapi-init-commands.c: In function ‘qmp_init_marshal’:
>     qapi/qapi-init-commands.c:70:84: error: ‘QAPI_FEATURE_ALLOW_WRITE_ONLY_OVERLAY’ undeclared (first use in this function)
>        70 |                          qmp_marshal_blockdev_snapshot, QCO_ALLOW_PRECONFIG, 1u << QAPI_FEATURE_ALLOW_WRITE_ONLY_OVERLAY);
>           |                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     qapi/qapi-init-commands.c:70:84: note: each undeclared identifier is reported only once for each function it appears in
>     qapi/qapi-init-commands.c:464:70: error: ‘QAPI_FEATURE_SAVEVM_MONITOR_NODES’ undeclared (first use in this function)
>       464 |                          qmp_marshal_human_monitor_command, 0, 1u << QAPI_FEATURE_SAVEVM_MONITOR_NODES);
>           |                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

IIUC, you are doing an patch-wise build. I see this error whe nbuilding
at patch 5, and its fixed by patch 6. These two patches have to be
squashed together really.

With regards,
Daniel
Markus Armbruster Aug. 8, 2024, 12:11 p.m. UTC | #3 
Daniel P. Berrangé <berrange@redhat.com> writes:

> This removed the QapiFeatures enum and auto-generates an enum which
> exposes all features defined by the schema to code.
>
> The 'deprecated' and 'unstable' features still have a little bit of
> special handling, being force defined to be the 1st + 2nd features
> in the enum, regardless of whether they're used in the schema. This
> is because QAPI common code references these features.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/qapi/util.h      |   5 --
>  meson.build              |   1 +
>  scripts/qapi/features.py | 134 +++++++++++++++++++++++++++++++++++++++
>  scripts/qapi/main.py     |   2 +
>  scripts/qapi/schema.py   |   5 +-
>  scripts/qapi/types.py    |   4 +-
>  6 files changed, 144 insertions(+), 7 deletions(-)
>  create mode 100644 scripts/qapi/features.py
>
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index a693cac9ea..9e390486c0 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -11,11 +11,6 @@
>  #ifndef QAPI_UTIL_H
>  #define QAPI_UTIL_H
>  
> -typedef enum {
> -    QAPI_FEATURE_DEPRECATED,
> -    QAPI_FEATURE_UNSTABLE,
> -} QapiFeature;
> -
>  typedef struct QEnumLookup {
>      const char *const *array;
>      const uint64_t *const features;
> diff --git a/meson.build b/meson.build
> index 97f63aa86c..40002c59f5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3268,6 +3268,7 @@ qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py',
>                       meson.current_source_dir() / 'scripts/qapi/schema.py',
>                       meson.current_source_dir() / 'scripts/qapi/source.py',
>                       meson.current_source_dir() / 'scripts/qapi/types.py',
> +                     meson.current_source_dir() / 'scripts/qapi/features.py',
>                       meson.current_source_dir() / 'scripts/qapi/visit.py',
>                       meson.current_source_dir() / 'scripts/qapi-gen.py'
>  ]
> diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
> new file mode 100644
> index 0000000000..9b77be6310
> --- /dev/null
> +++ b/scripts/qapi/features.py
> @@ -0,0 +1,134 @@
> +"""
> +QAPI types generator
> +
> +Copyright 2024 Red Hat
> +
> +This work is licensed under the terms of the GNU GPL, version 2.
> +# See the COPYING file in the top-level directory.
> +"""
> +
> +from typing import List, Optional
> +
> +from .common import c_enum_const, mcgen, c_name
> +from .gen import QAPISchemaMonolithicCVisitor
> +from .schema import (
> +    QAPISchema,
> +    QAPISchemaAlternatives,
> +    QAPISchemaBranches,
> +    QAPISchemaEntity,
> +    QAPISchemaEnumMember,
> +    QAPISchemaFeature,
> +    QAPISchemaIfCond,
> +    QAPISchemaObjectType,
> +    QAPISchemaObjectTypeMember,
> +    QAPISchemaType,
> +    QAPISchemaVariants,
> +)
> +from .source import QAPISourceInfo
> +
> +
> +class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
> +
> +    def __init__(self, prefix: str):
> +        super().__init__(
> +            prefix, 'qapi-features',
> +            ' * Schema-defined QAPI features',
> +            __doc__)
> +
> +        self.features = {}
> +
> +    def visit_end(self) -> None:
> +        features = []
> +
> +        # We always want special features to have the same
> +        # enum value across all schemas, since they're
> +        # referenced from common code. Put them at the
> +        # start of the list, regardless of whether they
> +        # are actually referenced in the schema
> +        for name in QAPISchemaFeature.SPECIAL_NAMES:
> +            features.append(name)
> +
> +        features.extend(sorted(self.features.keys()))
> +
> +        if len(features) > 64:
> +            raise Exception("Maximum of 64 schema features is permitted")

This is just one notch above assert len(features) > 64, so nope :)

Backends are not supposed to diagnose schema errors or constraint
violations.  That's the frontend's job.

Perhaps the simplest way to check this in the frontend is to build a
feature set in QAPISchema: initialize in .__init__(), update in
._make_features(), fail right there when asked to make a 64th.

> +
> +        self._genh.add("typedef enum {\n")
> +        for name in features:
> +            self._genh.add(f"    {c_enum_const(self._prefix + 'QAPI_FEATURE', name)},\n")

This duplicates part of gen.gen_features().  Suggest to factor out the
common part as gen_feature().

Note for later: enum QapiFeature is defined in generated
qapi-features.h.

> +
> +        self._genh.add("} " + c_name(self._prefix + 'QapiFeature') + ";\n")
> +
> +    def _record(self, features: List[QAPISchemaFeature]):
> +        for f in features:
> +            # Special features are handled separately
> +            if f.name in QAPISchemaFeature.SPECIAL_NAMES:
> +                continue
> +            self.features[f.name] = True
> +
> +    def visit_enum_type(self,
> +                        name: str,
> +                        info: Optional[QAPISourceInfo],
> +                        ifcond: QAPISchemaIfCond,
> +                        features: List[QAPISchemaFeature],
> +                        members: List[QAPISchemaEnumMember],
> +                        prefix: Optional[str]) -> None:
> +        self._record(features)
> +
> +    def visit_object_type_flat(self,
> +                               name: str,
> +                               info: Optional[QAPISourceInfo],
> +                               ifcond: QAPISchemaIfCond,
> +                               features: List[QAPISchemaFeature],
> +                               members: List[QAPISchemaObjectTypeMember],
> +                               branches: Optional[QAPISchemaBranches]) -> None:
> +        self._record(features)
> +
> +    def visit_object_type(self,
> +                          name: str,
> +                          info: Optional[QAPISourceInfo],
> +                          ifcond: QAPISchemaIfCond,
> +                          features: List[QAPISchemaFeature],
> +                          base: Optional[QAPISchemaObjectType],
> +                          members: List[QAPISchemaObjectTypeMember],
> +                          branches: Optional[QAPISchemaBranches]) -> None:
> +        self._record(features)
> +
> +    def visit_alternate_type(self,
> +                             name: str,
> +                             info: Optional[QAPISourceInfo],
> +                             ifcond: QAPISchemaIfCond,
> +                             features: List[QAPISchemaFeature],
> +                             alternatives: QAPISchemaAlternatives) -> None:
> +        self._record(features)
> +
> +    def visit_command(self,
> +                      name: str,
> +                      info: Optional[QAPISourceInfo],
> +                      ifcond: QAPISchemaIfCond,
> +                      features: List[QAPISchemaFeature],
> +                      arg_type: Optional[QAPISchemaObjectType],
> +                      ret_type: Optional[QAPISchemaType],
> +                      gen: bool,
> +                      success_response: bool,
> +                      boxed: bool,
> +                      allow_oob: bool,
> +                      allow_preconfig: bool,
> +                      coroutine: bool) -> None:
> +        self._record(features)
> +
> +    def visit_event(self,
> +                    name: str,
> +                    info: Optional[QAPISourceInfo],
> +                    ifcond: QAPISchemaIfCond,
> +                    features: List[QAPISchemaFeature],
> +                    arg_type: Optional[QAPISchemaObjectType],
> +                    boxed: bool) -> None:
> +        self._record(features)
> +

pycodestyle-3 gripes

    scripts/qapi/features.py:129:1: E302 expected 2 blank lines, found 1

> +def gen_features(schema: QAPISchema,
> +                 output_dir: str,
> +                 prefix: str) -> None:
> +    vis = QAPISchemaGenFeatureVisitor(prefix)
> +    schema.visit(vis)
> +    vis.write(output_dir)

We have another gen_features() in gen.py.  Not a show stopper, but could
we find equally good names that don't clash?

> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index 316736b6a2..2b9a2c0c02 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -18,6 +18,7 @@
>  from .introspect import gen_introspect
>  from .schema import QAPISchema
>  from .types import gen_types
> +from .features import gen_features
>  from .visit import gen_visit
>  
>  
> @@ -49,6 +50,7 @@ def generate(schema_file: str,
>  
>      schema = QAPISchema(schema_file)
>      gen_types(schema, output_dir, prefix, builtins)
> +    gen_features(schema, output_dir, prefix)
>      gen_visit(schema, output_dir, prefix, builtins)
>      gen_commands(schema, output_dir, prefix, gen_tracing)
>      gen_events(schema, output_dir, prefix)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index d65c35f6ee..160ce0a7c0 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -933,8 +933,11 @@ def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
>  class QAPISchemaFeature(QAPISchemaMember):
>      role = 'feature'
>  
> +    # Features which are standardized across all schemas
> +    SPECIAL_NAMES = ['deprecated', 'unstable']
> +
>      def is_special(self) -> bool:
> -        return self.name in ('deprecated', 'unstable')
> +        return self.name in QAPISchemaFeature.SPECIAL_NAMES
>  
>  
>  class QAPISchemaObjectTypeMember(QAPISchemaMember):
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index b2d26c2ea8..3435f1b0b0 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -313,7 +313,9 @@ def _begin_user_module(self, name: str) -> None:
>                                        types=types, visit=visit))
>          self._genh.preamble_add(mcgen('''
>  #include "qapi/qapi-builtin-types.h"
> -'''))
> +#include "%(prefix)sqapi-features.h"

This works, because it pulls in qapi-features.h basically everywhere.

It's actually needed only where we use enum QapiFeature.  So far, we
only ever generate uses with gen.gen_features().  Callers:

* gen_register_command() for qapi-init-commands.c.

* gen_enum_lookup() for qapi-types*.c and qapi-emit-events.c.

* gen_visit_object_members() for qapi-visit*.c.

Please include it just in these generated .c files.

> +''',
> +                                      prefix=self._prefix))
>  
>      def visit_begin(self, schema: QAPISchema) -> None:
>          # gen_object() is recursive, ensure it doesn't visit the empty type
diff mbox series

Patch

diff --git a/include/qapi/util.h b/include/qapi/util.h
index a693cac9ea..9e390486c0 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,11 +11,6 @@ 
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
-typedef enum {
-    QAPI_FEATURE_DEPRECATED,
-    QAPI_FEATURE_UNSTABLE,
-} QapiFeature;
-
 typedef struct QEnumLookup {
     const char *const *array;
     const uint64_t *const features;
diff --git a/meson.build b/meson.build
index 97f63aa86c..40002c59f5 100644
--- a/meson.build
+++ b/meson.build
@@ -3268,6 +3268,7 @@  qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py',
                      meson.current_source_dir() / 'scripts/qapi/schema.py',
                      meson.current_source_dir() / 'scripts/qapi/source.py',
                      meson.current_source_dir() / 'scripts/qapi/types.py',
+                     meson.current_source_dir() / 'scripts/qapi/features.py',
                      meson.current_source_dir() / 'scripts/qapi/visit.py',
                      meson.current_source_dir() / 'scripts/qapi-gen.py'
 ]
diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
new file mode 100644
index 0000000000..9b77be6310
--- /dev/null
+++ b/scripts/qapi/features.py
@@ -0,0 +1,134 @@ 
+"""
+QAPI types generator
+
+Copyright 2024 Red Hat
+
+This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+"""
+
+from typing import List, Optional
+
+from .common import c_enum_const, mcgen, c_name
+from .gen import QAPISchemaMonolithicCVisitor
+from .schema import (
+    QAPISchema,
+    QAPISchemaAlternatives,
+    QAPISchemaBranches,
+    QAPISchemaEntity,
+    QAPISchemaEnumMember,
+    QAPISchemaFeature,
+    QAPISchemaIfCond,
+    QAPISchemaObjectType,
+    QAPISchemaObjectTypeMember,
+    QAPISchemaType,
+    QAPISchemaVariants,
+)
+from .source import QAPISourceInfo
+
+
+class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
+
+    def __init__(self, prefix: str):
+        super().__init__(
+            prefix, 'qapi-features',
+            ' * Schema-defined QAPI features',
+            __doc__)
+
+        self.features = {}
+
+    def visit_end(self) -> None:
+        features = []
+
+        # We always want special features to have the same
+        # enum value across all schemas, since they're
+        # referenced from common code. Put them at the
+        # start of the list, regardless of whether they
+        # are actually referenced in the schema
+        for name in QAPISchemaFeature.SPECIAL_NAMES:
+            features.append(name)
+
+        features.extend(sorted(self.features.keys()))
+
+        if len(features) > 64:
+            raise Exception("Maximum of 64 schema features is permitted")
+
+        self._genh.add("typedef enum {\n")
+        for name in features:
+            self._genh.add(f"    {c_enum_const(self._prefix + 'QAPI_FEATURE', name)},\n")
+
+        self._genh.add("} " + c_name(self._prefix + 'QapiFeature') + ";\n")
+
+    def _record(self, features: List[QAPISchemaFeature]):
+        for f in features:
+            # Special features are handled separately
+            if f.name in QAPISchemaFeature.SPECIAL_NAMES:
+                continue
+            self.features[f.name] = True
+
+    def visit_enum_type(self,
+                        name: str,
+                        info: Optional[QAPISourceInfo],
+                        ifcond: QAPISchemaIfCond,
+                        features: List[QAPISchemaFeature],
+                        members: List[QAPISchemaEnumMember],
+                        prefix: Optional[str]) -> None:
+        self._record(features)
+
+    def visit_object_type_flat(self,
+                               name: str,
+                               info: Optional[QAPISourceInfo],
+                               ifcond: QAPISchemaIfCond,
+                               features: List[QAPISchemaFeature],
+                               members: List[QAPISchemaObjectTypeMember],
+                               branches: Optional[QAPISchemaBranches]) -> None:
+        self._record(features)
+
+    def visit_object_type(self,
+                          name: str,
+                          info: Optional[QAPISourceInfo],
+                          ifcond: QAPISchemaIfCond,
+                          features: List[QAPISchemaFeature],
+                          base: Optional[QAPISchemaObjectType],
+                          members: List[QAPISchemaObjectTypeMember],
+                          branches: Optional[QAPISchemaBranches]) -> None:
+        self._record(features)
+
+    def visit_alternate_type(self,
+                             name: str,
+                             info: Optional[QAPISourceInfo],
+                             ifcond: QAPISchemaIfCond,
+                             features: List[QAPISchemaFeature],
+                             alternatives: QAPISchemaAlternatives) -> None:
+        self._record(features)
+
+    def visit_command(self,
+                      name: str,
+                      info: Optional[QAPISourceInfo],
+                      ifcond: QAPISchemaIfCond,
+                      features: List[QAPISchemaFeature],
+                      arg_type: Optional[QAPISchemaObjectType],
+                      ret_type: Optional[QAPISchemaType],
+                      gen: bool,
+                      success_response: bool,
+                      boxed: bool,
+                      allow_oob: bool,
+                      allow_preconfig: bool,
+                      coroutine: bool) -> None:
+        self._record(features)
+
+    def visit_event(self,
+                    name: str,
+                    info: Optional[QAPISourceInfo],
+                    ifcond: QAPISchemaIfCond,
+                    features: List[QAPISchemaFeature],
+                    arg_type: Optional[QAPISchemaObjectType],
+                    boxed: bool) -> None:
+        self._record(features)
+
+def gen_features(schema: QAPISchema,
+                 output_dir: str,
+                 prefix: str) -> None:
+    vis = QAPISchemaGenFeatureVisitor(prefix)
+    schema.visit(vis)
+    vis.write(output_dir)
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 316736b6a2..2b9a2c0c02 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -18,6 +18,7 @@ 
 from .introspect import gen_introspect
 from .schema import QAPISchema
 from .types import gen_types
+from .features import gen_features
 from .visit import gen_visit
 
 
@@ -49,6 +50,7 @@  def generate(schema_file: str,
 
     schema = QAPISchema(schema_file)
     gen_types(schema, output_dir, prefix, builtins)
+    gen_features(schema, output_dir, prefix)
     gen_visit(schema, output_dir, prefix, builtins)
     gen_commands(schema, output_dir, prefix, gen_tracing)
     gen_events(schema, output_dir, prefix)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d65c35f6ee..160ce0a7c0 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -933,8 +933,11 @@  def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
 class QAPISchemaFeature(QAPISchemaMember):
     role = 'feature'
 
+    # Features which are standardized across all schemas
+    SPECIAL_NAMES = ['deprecated', 'unstable']
+
     def is_special(self) -> bool:
-        return self.name in ('deprecated', 'unstable')
+        return self.name in QAPISchemaFeature.SPECIAL_NAMES
 
 
 class QAPISchemaObjectTypeMember(QAPISchemaMember):
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index b2d26c2ea8..3435f1b0b0 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -313,7 +313,9 @@  def _begin_user_module(self, name: str) -> None:
                                       types=types, visit=visit))
         self._genh.preamble_add(mcgen('''
 #include "qapi/qapi-builtin-types.h"
-'''))
+#include "%(prefix)sqapi-features.h"
+''',
+                                      prefix=self._prefix))
 
     def visit_begin(self, schema: QAPISchema) -> None:
         # gen_object() is recursive, ensure it doesn't visit the empty type