[PATCH 0/6] qapi: generalize special features

[PATCH 0/6] qapi: generalize special features

[Daniel P. Berrangé]

This series is a spin-off from

  https://lists.nongnu.org/archive/html/qemu-devel/2024-06/msg00807.html

That series introduced a pragma allowing a schema to declare extra
features that would be exposed to code.

Following Markus' suggestion:

  https://lists.nongnu.org/archive/html/qemu-devel/2024-07/msg03765.html

I've changed impl such that we expose all features to the code
regardless of whether they are special, and don't require any pragma.

I've split it from the QGA patches since it makes more sense to work
on this bit in isolation.

Daniel P. Berrangé (6):
  qapi: change 'unsigned special_features' to 'uint64_t features'
  scripts/qapi: rename 'special_features' to 'features'
  qapi: use "QAPI_FEATURE" as namespace for features
  qapi: cope with  feature names containing a '-'
  qapi: apply schema prefix to QAPI feature enum constants
  qapi: expose all schema features to code

 include/qapi/compat-policy.h  |   2 +-
 include/qapi/qmp/dispatch.h   |   4 +-
 include/qapi/util.h           |   7 +-
 include/qapi/visitor-impl.h   |   4 +-
 include/qapi/visitor.h        |  12 +--
 meson.build                   |   1 +
 qapi/qapi-forward-visitor.c   |   8 +-
 qapi/qapi-util.c              |   6 +-
 qapi/qapi-visit-core.c        |  12 +--
 qapi/qmp-dispatch.c           |   2 +-
 qapi/qmp-registry.c           |   4 +-
 qapi/qobject-input-visitor.c  |   4 +-
 qapi/qobject-output-visitor.c |   6 +-
 scripts/qapi/commands.py      |   9 ++-
 scripts/qapi/events.py        |   3 +-
 scripts/qapi/features.py      | 134 ++++++++++++++++++++++++++++++++++
 scripts/qapi/gen.py           |   9 ++-
 scripts/qapi/main.py          |   2 +
 scripts/qapi/schema.py        |   5 +-
 scripts/qapi/types.py         |  19 +++--
 scripts/qapi/visit.py         |  17 +++--
 21 files changed, 206 insertions(+), 64 deletions(-)
 create mode 100644 scripts/qapi/features.py

-- 
2.45.2

[PATCH 1/6] qapi: change 'unsigned special_features' to 'uint64_t features'

[Daniel P. Berrangé]

The "special_features" field / parameter holds the subset of schema
features that are for internal code use. Specifically 'DEPRECATED'
and 'UNSTABLE'.

This special casing of internal features is going to be removed, so
prepare for that by renaming to 'features'. Using a fixed size type
is also best practice for bit fields.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/compat-policy.h  |  2 +-
 include/qapi/qmp/dispatch.h   |  4 ++--
 include/qapi/util.h           |  4 ++--
 include/qapi/visitor-impl.h   |  4 ++--
 include/qapi/visitor.h        | 12 ++++++------
 qapi/qapi-forward-visitor.c   |  8 ++++----
 qapi/qapi-util.c              |  6 +++---
 qapi/qapi-visit-core.c        | 12 ++++++------
 qapi/qmp-dispatch.c           |  2 +-
 qapi/qmp-registry.c           |  4 ++--
 qapi/qobject-input-visitor.c  |  4 ++--
 qapi/qobject-output-visitor.c |  6 +++---
 scripts/qapi/types.py         |  2 +-
 13 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index 8b7b25c0b5..ea65e10744 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -18,7 +18,7 @@
 
 extern CompatPolicy compat_policy;
 
-bool compat_policy_input_ok(unsigned special_features,
+bool compat_policy_input_ok(uint64_t features,
                             const CompatPolicy *policy,
                             ErrorClass error_class,
                             const char *kind, const char *name,
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index f2e956813a..e0ee1ad3ac 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -33,7 +33,7 @@ typedef struct QmpCommand
     /* Runs in coroutine context if QCO_COROUTINE is set */
     QmpCommandFunc *fn;
     QmpCommandOptions options;
-    unsigned special_features;
+    uint64_t features;
     QTAILQ_ENTRY(QmpCommand) node;
     bool enabled;
     const char *disable_reason;
@@ -43,7 +43,7 @@ typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options,
-                          unsigned special_features);
+                          uint64_t features);
 const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
                                    const char *name);
 void qmp_disable_command(QmpCommandList *cmds, const char *name,
diff --git a/include/qapi/util.h b/include/qapi/util.h
index b8254247b8..b377cb9c70 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -14,11 +14,11 @@
 typedef enum {
     QAPI_DEPRECATED,
     QAPI_UNSTABLE,
-} QapiSpecialFeature;
+} QapiFeature;
 
 typedef struct QEnumLookup {
     const char *const *array;
-    const unsigned char *const special_features;
+    const uint64_t *const features;
     const int size;
 } QEnumLookup;
 
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 2badec5ba4..7beb0dbfa5 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -115,11 +115,11 @@ struct Visitor
 
     /* Optional */
     bool (*policy_reject)(Visitor *v, const char *name,
-                          unsigned special_features, Error **errp);
+                          uint64_t features, Error **errp);
 
     /* Optional */
     bool (*policy_skip)(Visitor *v, const char *name,
-                        unsigned special_features);
+                        uint64_t features);
 
     /* Must be set */
     VisitorType type;
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 27b85d4700..f6a9b0743f 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -463,29 +463,29 @@ bool visit_optional(Visitor *v, const char *name, bool *present);
 /*
  * Should we reject member @name due to policy?
  *
- * @special_features is the member's special features encoded as a
- * bitset of QapiSpecialFeature.
+ * @features is the member's special features encoded as a
+ * bitset of QapiFeature.
  *
  * @name must not be NULL.  This function is only useful between
  * visit_start_struct() and visit_end_struct(), since only objects
  * have deprecated members.
  */
 bool visit_policy_reject(Visitor *v, const char *name,
-                         unsigned special_features, Error **errp);
+                         uint64_t features, Error **errp);
 
 /*
  *
  * Should we skip member @name due to policy?
  *
- * @special_features is the member's special features encoded as a
- * bitset of QapiSpecialFeature.
+ * @features is the member's special features encoded as a
+ * bitset of QapiFeature.
  *
  * @name must not be NULL.  This function is only useful between
  * visit_start_struct() and visit_end_struct(), since only objects
  * have deprecated members.
  */
 bool visit_policy_skip(Visitor *v, const char *name,
-                       unsigned special_features);
+                       uint64_t features);
 
 /*
  * Set policy for handling deprecated management interfaces.
diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
index e36d9bc9ba..6e9a784a9f 100644
--- a/qapi/qapi-forward-visitor.c
+++ b/qapi/qapi-forward-visitor.c
@@ -246,7 +246,7 @@ static void forward_field_optional(Visitor *v, const char *name, bool *present)
 }
 
 static bool forward_field_policy_reject(Visitor *v, const char *name,
-                                        unsigned special_features,
+                                        uint64_t features,
                                         Error **errp)
 {
     ForwardFieldVisitor *ffv = to_ffv(v);
@@ -254,18 +254,18 @@ static bool forward_field_policy_reject(Visitor *v, const char *name,
     if (!forward_field_translate_name(ffv, &name, errp)) {
         return true;
     }
-    return visit_policy_reject(ffv->target, name, special_features, errp);
+    return visit_policy_reject(ffv->target, name, features, errp);
 }
 
 static bool forward_field_policy_skip(Visitor *v, const char *name,
-                                      unsigned special_features)
+                                      uint64_t features)
 {
     ForwardFieldVisitor *ffv = to_ffv(v);
 
     if (!forward_field_translate_name(ffv, &name, NULL)) {
         return true;
     }
-    return visit_policy_skip(ffv->target, name, special_features);
+    return visit_policy_skip(ffv->target, name, features);
 }
 
 static void forward_field_complete(Visitor *v, void *opaque)
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 65a7d18437..3d849fe034 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -37,19 +37,19 @@ static bool compat_policy_input_ok1(const char *adjective,
     }
 }
 
-bool compat_policy_input_ok(unsigned special_features,
+bool compat_policy_input_ok(uint64_t features,
                             const CompatPolicy *policy,
                             ErrorClass error_class,
                             const char *kind, const char *name,
                             Error **errp)
 {
-    if ((special_features & 1u << QAPI_DEPRECATED)
+    if ((features & 1u << QAPI_DEPRECATED)
         && !compat_policy_input_ok1("Deprecated",
                                     policy->deprecated_input,
                                     error_class, kind, name, errp)) {
         return false;
     }
-    if ((special_features & (1u << QAPI_UNSTABLE))
+    if ((features & (1u << QAPI_UNSTABLE))
         && !compat_policy_input_ok1("Unstable",
                                     policy->unstable_input,
                                     error_class, kind, name, errp)) {
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6c13510a2b..706c61e026 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -141,21 +141,21 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
 }
 
 bool visit_policy_reject(Visitor *v, const char *name,
-                         unsigned special_features, Error **errp)
+                         uint64_t features, Error **errp)
 {
     trace_visit_policy_reject(v, name);
     if (v->policy_reject) {
-        return v->policy_reject(v, name, special_features, errp);
+        return v->policy_reject(v, name, features, errp);
     }
     return false;
 }
 
 bool visit_policy_skip(Visitor *v, const char *name,
-                       unsigned special_features)
+                       uint64_t features)
 {
     trace_visit_policy_skip(v, name);
     if (v->policy_skip) {
-        return v->policy_skip(v, name, special_features);
+        return v->policy_skip(v, name, features);
     }
     return false;
 }
@@ -409,8 +409,8 @@ static bool input_type_enum(Visitor *v, const char *name, int *obj,
         return false;
     }
 
-    if (lookup->special_features
-        && !compat_policy_input_ok(lookup->special_features[value],
+    if (lookup->features
+        && !compat_policy_input_ok(lookup->features[value],
                                    &v->compat_policy,
                                    ERROR_CLASS_GENERIC_ERROR,
                                    "value", enum_str, errp)) {
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 176b549473..d411eebf4e 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -173,7 +173,7 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
                   "The command %s has not been found", command);
         goto out;
     }
-    if (!compat_policy_input_ok(cmd->special_features, &compat_policy,
+    if (!compat_policy_input_ok(cmd->features, &compat_policy,
                                 ERROR_CLASS_COMMAND_NOT_FOUND,
                                 "command", command, &err)) {
         goto out;
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 485bc5e6fc..bfcabec526 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -17,7 +17,7 @@
 
 void qmp_register_command(QmpCommandList *cmds, const char *name,
                           QmpCommandFunc *fn, QmpCommandOptions options,
-                          unsigned special_features)
+                          uint64_t features)
 {
     QmpCommand *cmd = g_malloc0(sizeof(*cmd));
 
@@ -28,7 +28,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
     cmd->fn = fn;
     cmd->enabled = true;
     cmd->options = options;
-    cmd->special_features = special_features;
+    cmd->features = features;
     QTAILQ_INSERT_TAIL(cmds, cmd, node);
 }
 
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f110a804b2..ff9c726de3 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -664,10 +664,10 @@ static void qobject_input_optional(Visitor *v, const char *name, bool *present)
 }
 
 static bool qobject_input_policy_reject(Visitor *v, const char *name,
-                                        unsigned special_features,
+                                        uint64_t features,
                                         Error **errp)
 {
-    return !compat_policy_input_ok(special_features, &v->compat_policy,
+    return !compat_policy_input_ok(features, &v->compat_policy,
                                    ERROR_CLASS_GENERIC_ERROR,
                                    "parameter", name, errp);
 }
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 74770edd73..8902287caa 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -210,13 +210,13 @@ static bool qobject_output_type_null(Visitor *v, const char *name,
 }
 
 static bool qobject_output_policy_skip(Visitor *v, const char *name,
-                                       unsigned special_features)
+                                       uint64_t features)
 {
     CompatPolicy *pol = &v->compat_policy;
 
-    return ((special_features & 1u << QAPI_DEPRECATED)
+    return ((features & 1u << QAPI_DEPRECATED)
             && pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE)
-        || ((special_features & 1u << QAPI_UNSTABLE)
+        || ((features & 1u << QAPI_UNSTABLE)
             && pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE);
 }
 
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 0dd0b00ada..7bc3f8241f 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -71,7 +71,7 @@ def gen_enum_lookup(name: str,
     if feats:
         ret += mcgen('''
     },
-    .special_features = (const unsigned char[%(max_index)s]) {
+    .features = (const uint64_t[%(max_index)s]) {
 ''',
                      max_index=max_index)
         ret += feats
-- 
2.45.2

[PATCH 2/6] scripts/qapi: rename 'special_features' to 'features'

[Daniel P. Berrangé]

This updates the QAPI code generation to refer to 'features' instead
of 'special_features', in preparation for generalizing their exposure.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/commands.py |  4 ++--
 scripts/qapi/gen.py      |  8 ++++----
 scripts/qapi/types.py    | 10 +++++-----
 scripts/qapi/visit.py    | 14 +++++++-------
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 79951a841f..d629d2d97e 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -25,7 +25,7 @@
     QAPIGenC,
     QAPISchemaModularCVisitor,
     build_params,
-    gen_special_features,
+    gen_features,
     ifcontext,
 )
 from .schema import (
@@ -298,7 +298,7 @@ def gen_register_command(name: str,
 ''',
                 name=name, c_name=c_name(name),
                 opts=' | '.join(options) or 0,
-                feats=gen_special_features(features))
+                feats=gen_features(features))
     return ret
 
 
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 6a8abe0041..e6c80cce23 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -40,10 +40,10 @@
 from .source import QAPISourceInfo
 
 
-def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
-    special_features = [f"1u << QAPI_{feat.name.upper()}"
-                        for feat in features if feat.is_special()]
-    return ' | '.join(special_features) or '0'
+def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
+    features = [f"1u << QAPI_{feat.name.upper()}"
+                for feat in features if feat.is_special()]
+    return ' | '.join(features) or '0'
 
 
 class QAPIGen:
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 7bc3f8241f..ade6b7a3d7 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -18,7 +18,7 @@
 from .common import c_enum_const, c_name, mcgen
 from .gen import (
     QAPISchemaModularCVisitor,
-    gen_special_features,
+    gen_features,
     ifcontext,
 )
 from .schema import (
@@ -61,12 +61,12 @@ def gen_enum_lookup(name: str,
                      index=index, name=memb.name)
         ret += memb.ifcond.gen_endif()
 
-        special_features = gen_special_features(memb.features)
-        if special_features != '0':
+        features = gen_features(memb.features)
+        if features != '0':
             feats += mcgen('''
-        [%(index)s] = %(special_features)s,
+        [%(index)s] = %(features)s,
 ''',
-                           index=index, special_features=special_features)
+                           index=index, features=features)
 
     if feats:
         ret += mcgen('''
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 12f92e429f..8dbf4ef1c3 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -23,7 +23,7 @@
 )
 from .gen import (
     QAPISchemaModularCVisitor,
-    gen_special_features,
+    gen_features,
     ifcontext,
 )
 from .schema import (
@@ -103,15 +103,15 @@ def gen_visit_object_members(name: str,
 ''',
                          name=memb.name, has=has)
             indent.increase()
-        special_features = gen_special_features(memb.features)
-        if special_features != '0':
+        features = gen_features(memb.features)
+        if features != '0':
             ret += mcgen('''
-    if (visit_policy_reject(v, "%(name)s", %(special_features)s, errp)) {
+    if (visit_policy_reject(v, "%(name)s", %(features)s, errp)) {
         return false;
     }
-    if (!visit_policy_skip(v, "%(name)s", %(special_features)s)) {
+    if (!visit_policy_skip(v, "%(name)s", %(features)s)) {
 ''',
-                         name=memb.name, special_features=special_features)
+                         name=memb.name, features=features)
             indent.increase()
         ret += mcgen('''
     if (!visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, errp)) {
@@ -120,7 +120,7 @@ def gen_visit_object_members(name: str,
 ''',
                      c_type=memb.type.c_name(), name=memb.name,
                      c_name=c_name(memb.name))
-        if special_features != '0':
+        if features != '0':
             indent.decrease()
             ret += mcgen('''
     }
-- 
2.45.2

[PATCH 3/6] qapi: use "QAPI_FEATURE" as namespace for features

[Daniel P. Berrangé]

This more clearly distinguishes the feature constants from other
QAPI constants.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/qapi/util.h           | 4 ++--
 qapi/qapi-util.c              | 4 ++--
 qapi/qobject-output-visitor.c | 4 ++--
 scripts/qapi/gen.py           | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index b377cb9c70..a693cac9ea 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -12,8 +12,8 @@
 #define QAPI_UTIL_H
 
 typedef enum {
-    QAPI_DEPRECATED,
-    QAPI_UNSTABLE,
+    QAPI_FEATURE_DEPRECATED,
+    QAPI_FEATURE_UNSTABLE,
 } QapiFeature;
 
 typedef struct QEnumLookup {
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 3d849fe034..48c8c867d3 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -43,13 +43,13 @@ bool compat_policy_input_ok(uint64_t features,
                             const char *kind, const char *name,
                             Error **errp)
 {
-    if ((features & 1u << QAPI_DEPRECATED)
+    if ((features & 1u << QAPI_FEATURE_DEPRECATED)
         && !compat_policy_input_ok1("Deprecated",
                                     policy->deprecated_input,
                                     error_class, kind, name, errp)) {
         return false;
     }
-    if ((features & (1u << QAPI_UNSTABLE))
+    if ((features & (1u << QAPI_FEATURE_UNSTABLE))
         && !compat_policy_input_ok1("Unstable",
                                     policy->unstable_input,
                                     error_class, kind, name, errp)) {
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 8902287caa..b13c94d913 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -214,9 +214,9 @@ static bool qobject_output_policy_skip(Visitor *v, const char *name,
 {
     CompatPolicy *pol = &v->compat_policy;
 
-    return ((features & 1u << QAPI_DEPRECATED)
+    return ((features & 1u << QAPI_FEATURE_DEPRECATED)
             && pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE)
-        || ((features & 1u << QAPI_UNSTABLE)
+        || ((features & 1u << QAPI_FEATURE_UNSTABLE)
             && pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE);
 }
 
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index e6c80cce23..0ff29dc776 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -41,7 +41,7 @@
 
 
 def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
-    features = [f"1u << QAPI_{feat.name.upper()}"
+    features = [f"1u << QAPI_FEATURE_{feat.name.upper()}"
                 for feat in features if feat.is_special()]
     return ' | '.join(features) or '0'
 
-- 
2.45.2

[PATCH 4/6] qapi: cope with feature names containing a '-'

[Daniel P. Berrangé]

When we shortly expose all feature names to code, it will be valid to
include a '-', which must be translated to a '_' for the enum constants.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/gen.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 0ff29dc776..036977d989 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -24,6 +24,7 @@
 )
 
 from .common import (
+    c_enum_const,
     c_fname,
     c_name,
     guardend,
@@ -41,7 +42,7 @@
 
 
 def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
-    features = [f"1u << QAPI_FEATURE_{feat.name.upper()}"
+    features = [f"1u << {c_enum_const('QAPI_FEATURE', feat.name)}"
                 for feat in features if feat.is_special()]
     return ' | '.join(features) or '0'
 
-- 
2.45.2

[PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Daniel P. Berrangé]

This allows us to include multiple QAPI schemas in the same file.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/commands.py | 7 ++++---
 scripts/qapi/events.py   | 3 ++-
 scripts/qapi/gen.py      | 6 +++---
 scripts/qapi/types.py    | 5 +++--
 scripts/qapi/visit.py    | 5 +++--
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index d629d2d97e..81134de6c0 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -280,7 +280,8 @@ def gen_register_command(name: str,
                          success_response: bool,
                          allow_oob: bool,
                          allow_preconfig: bool,
-                         coroutine: bool) -> str:
+                         coroutine: bool,
+                         prefix: str) -> str:
     options = []
 
     if not success_response:
@@ -298,7 +299,7 @@ def gen_register_command(name: str,
 ''',
                 name=name, c_name=c_name(name),
                 opts=' | '.join(options) or 0,
-                feats=gen_features(features))
+                feats=gen_features(prefix, features))
     return ret
 
 
@@ -407,7 +408,7 @@ def visit_command(self,
             with ifcontext(ifcond, self._genh, self._genc):
                 self._genc.add(gen_register_command(
                     name, features, success_response, allow_oob,
-                    allow_preconfig, coroutine))
+                    allow_preconfig, coroutine, self._prefix))
 
 
 def gen_commands(schema: QAPISchema,
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index d1f639981a..8a489e559a 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -218,7 +218,8 @@ def visit_end(self) -> None:
         self._genh.add(gen_enum(self._event_enum_name,
                                 self._event_enum_members))
         self._genc.add(gen_enum_lookup(self._event_enum_name,
-                                       self._event_enum_members))
+                                       self._event_enum_members,
+                                       self._prefix))
         self._genh.add(mcgen('''
 
 void %(event_emit)s(%(event_enum)s event, QDict *qdict);
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 036977d989..399a0ae62d 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -41,9 +41,9 @@
 from .source import QAPISourceInfo
 
 
-def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
-    features = [f"1u << {c_enum_const('QAPI_FEATURE', feat.name)}"
-                for feat in features if feat.is_special()]
+def gen_features(prefix, features: Sequence[QAPISchemaFeature]) -> str:
+    features = [f"1u << {c_enum_const(prefix + 'QAPI_FEATURE', feat.name)}"
+                for feat in features]
     return ' | '.join(features) or '0'
 
 
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index ade6b7a3d7..b2d26c2ea8 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -43,6 +43,7 @@
 
 def gen_enum_lookup(name: str,
                     members: List[QAPISchemaEnumMember],
+                    modprefix: str,
                     prefix: Optional[str] = None) -> str:
     max_index = c_enum_const(name, '_MAX', prefix)
     feats = ''
@@ -61,7 +62,7 @@ def gen_enum_lookup(name: str,
                      index=index, name=memb.name)
         ret += memb.ifcond.gen_endif()
 
-        features = gen_features(memb.features)
+        features = gen_features(modprefix, memb.features)
         if features != '0':
             feats += mcgen('''
         [%(index)s] = %(features)s,
@@ -331,7 +332,7 @@ def visit_enum_type(self,
                         prefix: Optional[str]) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.preamble_add(gen_enum(name, members, prefix))
-            self._genc.add(gen_enum_lookup(name, members, prefix))
+            self._genc.add(gen_enum_lookup(name, members, self._prefix, prefix))
 
     def visit_array_type(self,
                          name: str,
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 8dbf4ef1c3..883d720e36 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -62,6 +62,7 @@ def gen_visit_members_decl(name: str) -> str:
 
 
 def gen_visit_object_members(name: str,
+                             prefix: str,
                              base: Optional[QAPISchemaObjectType],
                              members: List[QAPISchemaObjectTypeMember],
                              branches: Optional[QAPISchemaBranches]) -> str:
@@ -103,7 +104,7 @@ def gen_visit_object_members(name: str,
 ''',
                          name=memb.name, has=has)
             indent.increase()
-        features = gen_features(memb.features)
+        features = gen_features(prefix, memb.features)
         if features != '0':
             ret += mcgen('''
     if (visit_policy_reject(v, "%(name)s", %(features)s, errp)) {
@@ -402,7 +403,7 @@ def visit_object_type(self,
             return
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_visit_members_decl(name))
-            self._genc.add(gen_visit_object_members(name, base,
+            self._genc.add(gen_visit_object_members(name, self._prefix, base,
                                                     members, branches))
             # TODO Worth changing the visitor signature, so we could
             # directly use rather than repeat type.is_implicit()?
-- 
2.45.2

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

[Daniel P. Berrangé]

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")
+
+        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
-- 
2.45.2

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

[Markus Armbruster]

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);
          |                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/6] qapi: change 'unsigned special_features' to 'uint64_t features'

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The "special_features" field / parameter holds the subset of schema
> features that are for internal code use. Specifically 'DEPRECATED'
> and 'UNSTABLE'.
>
> This special casing of internal features is going to be removed, so
> prepare for that by renaming to 'features'. Using a fixed size type
> is also best practice for bit fields.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

[...]

>  static void forward_field_complete(Visitor *v, void *opaque)
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 65a7d18437..3d849fe034 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -37,19 +37,19 @@ static bool compat_policy_input_ok1(const char *adjective,
>      }
>  }
>  
> -bool compat_policy_input_ok(unsigned special_features,
> +bool compat_policy_input_ok(uint64_t features,
>                              const CompatPolicy *policy,
>                              ErrorClass error_class,
>                              const char *kind, const char *name,
>                              Error **errp)
>  {
> -    if ((special_features & 1u << QAPI_DEPRECATED)
> +    if ((features & 1u << QAPI_DEPRECATED)

Before the patch, both operands of & are unsigned.  The patch changes
one of them to uint64_t.  Shouldn't we change the other one, too?

>          && !compat_policy_input_ok1("Deprecated",
>                                      policy->deprecated_input,
>                                      error_class, kind, name, errp)) {
>          return false;
>      }
> -    if ((special_features & (1u << QAPI_UNSTABLE))
> +    if ((features & (1u << QAPI_UNSTABLE))

Likewise.

>          && !compat_policy_input_ok1("Unstable",
>                                      policy->unstable_input,
>                                      error_class, kind, name, errp)) {

[...]

> diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
> index 74770edd73..8902287caa 100644
> --- a/qapi/qobject-output-visitor.c
> +++ b/qapi/qobject-output-visitor.c
> @@ -210,13 +210,13 @@ static bool qobject_output_type_null(Visitor *v, const char *name,
>  }
>  
>  static bool qobject_output_policy_skip(Visitor *v, const char *name,
> -                                       unsigned special_features)
> +                                       uint64_t features)
>  {
>      CompatPolicy *pol = &v->compat_policy;
>  
> -    return ((special_features & 1u << QAPI_DEPRECATED)
> +    return ((features & 1u << QAPI_DEPRECATED)

Likewise.

>              && pol->deprecated_output == COMPAT_POLICY_OUTPUT_HIDE)
> -        || ((special_features & 1u << QAPI_UNSTABLE)
> +        || ((features & 1u << QAPI_UNSTABLE)

Likewise.

>              && pol->unstable_output == COMPAT_POLICY_OUTPUT_HIDE);
>  }
>  

[...]

Re: [PATCH 2/6] scripts/qapi: rename 'special_features' to 'features'

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This updates the QAPI code generation to refer to 'features' instead
> of 'special_features', in preparation for generalizing their exposure.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Have you considered squashing this into the previous patch?

[...]

> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 6a8abe0041..e6c80cce23 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -40,10 +40,10 @@
>  from .source import QAPISourceInfo
>  
>  
> -def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
> -    special_features = [f"1u << QAPI_{feat.name.upper()}"
> -                        for feat in features if feat.is_special()]
> -    return ' | '.join(special_features) or '0'
> +def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
> +    features = [f"1u << QAPI_{feat.name.upper()}"
> +                for feat in features if feat.is_special()]
> +    return ' | '.join(features) or '0'

This generates a bitwise or of unsigned operands.  Shouldn't we change
the operands to uint64_t?

>  
>  
>  class QAPIGen:

[...]

Re: [PATCH 3/6] qapi: use "QAPI_FEATURE" as namespace for features

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This more clearly distinguishes the feature constants from other
> QAPI constants.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 4/6] qapi: cope with feature names containing a '-'

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> When we shortly expose all feature names to code, it will be valid to
> include a '-', which must be translated to a '_' for the enum constants.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/gen.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index 0ff29dc776..036977d989 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -24,6 +24,7 @@
>  )
>  
>  from .common import (
> +    c_enum_const,
>      c_fname,
>      c_name,
>      guardend,
> @@ -41,7 +42,7 @@
>  
>  
>  def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
> -    features = [f"1u << QAPI_FEATURE_{feat.name.upper()}"
> +    features = [f"1u << {c_enum_const('QAPI_FEATURE', feat.name)}"
>                  for feat in features if feat.is_special()]
>      return ' | '.join(features) or '0'

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This allows us to include multiple QAPI schemas in the same file.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

I figure you had reason to simultaneously include headers generated for
multiple schemas.  Do tell :)

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Daniel P. Berrangé]

On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This allows us to include multiple QAPI schemas in the same file.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> I figure you had reason to simultaneously include headers generated for
> multiple schemas.  Do tell :)

I didn't want to have this patch, but the unit tests do this :-(

[2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o 
cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
In file included from tests/test-qapi-types-sub-sub-module.h:17,
                 from tests/test-qapi-visit-sub-sub-module.h:17,
                 from tests/test-qapi-commands-sub-sub-module.c:19:
tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
   16 |     QAPI_FEATURE_DEPRECATED,
      |     ^~~~~~~~~~~~~~~~~~~~~~~
In file included from ./qapi/qapi-types-error.h:17,
                 from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
                 from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
                 from tests/test-qapi-commands-sub-sub-module.c:14:
./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
   16 |     QAPI_FEATURE_DEPRECATED,
      |     ^~~~~~~~~~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:167: run-ninja] Error 1
make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
make: *** [GNUmakefile:6: build] Error 2



I would be nice to eliminate that, but some parts of the test appear
to pull in more of the system emulator code which forces in the main
qapi schema

n file included from ./qapi/qapi-types-block-core.h:17,
                 from /var/home/berrange/src/virt/qemu/include/block/block-common.h:27,
                 from /var/home/berrange/src/virt/qemu/include/block/block-global-state.h:27,
                 from /var/home/berrange/src/virt/qemu/include/block/block.h:27,
                 from /var/home/berrange/src/virt/qemu/include/monitor/monitor.h:4,
                 from /var/home/berrange/src/virt/qemu/include/qapi/qmp/dispatch.h:17,
                 from tests/test-qapi-init-commands.h:16,
                 from tests/test-qapi-init-commands.c:15:
./qapi/qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
   16 |     QAPI_FEATURE_DEPRECATED,
      |     ^~~~~~~~~~~~~~~~~~~~~~~
In file included from tests/include/../test-qapi-types-sub-sub-module.h:17,
                 from tests/include/../test-qapi-commands-sub-sub-module.h:16,
                 from tests/include/test-qapi-commands-sub-module.h:16,
                 from tests/test-qapi-commands.h:16,
                 from tests/test-qapi-init-commands.c:14:
tests/include/../test-qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
   16 |     QAPI_FEATURE_DEPRECATED,
      |     ^~~~~~~~~~~~~~~~~~~~~~~
tests/test-qapi-init-commands.c: In function ‘test_qmp_init_marshal’:
tests/test-qapi-init-commands.c:64:71: error: ‘TEST_QAPI_FEATURE_DEPRECATED’ undeclared (first use in this function); did you mean ‘QAPI_FEATURE_DEPRECATED’?
   64 |                          qmp_marshal_test_command_features1, 0, 1u << TEST_QAPI_FEATURE_DEPRECATED);
      |                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                       QAPI_FEATURE_DEPRECATED
tests/test-qapi-init-commands.c:64:71: note: each undeclared identifier is reported only once for each function it appears in


Maybe the test needs to be split into two ?  One test exclusively testing
with the tests/qapi-schema/qapi-schema-test.json, and one test exclusively
with the main QMP QAPI schema ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > This allows us to include multiple QAPI schemas in the same file.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> 
>> I figure you had reason to simultaneously include headers generated for
>> multiple schemas.  Do tell :)
>
> I didn't want to have this patch, but the unit tests do this :-(
>
> [2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
> FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o 
> cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
> In file included from tests/test-qapi-types-sub-sub-module.h:17,
>                  from tests/test-qapi-visit-sub-sub-module.h:17,
>                  from tests/test-qapi-commands-sub-sub-module.c:19:
> tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
>    16 |     QAPI_FEATURE_DEPRECATED,
>       |     ^~~~~~~~~~~~~~~~~~~~~~~
> In file included from ./qapi/qapi-types-error.h:17,
>                  from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
>                  from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
>                  from tests/test-qapi-commands-sub-sub-module.c:14:
> ./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
>    16 |     QAPI_FEATURE_DEPRECATED,
>       |     ^~~~~~~~~~~~~~~~~~~~~~~
> ninja: build stopped: subcommand failed.
> make[1]: *** [Makefile:167: run-ninja] Error 1
> make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
> make: *** [GNUmakefile:6: build] Error 2

Compiles for me with PATCH 5/6 taken out.  What am I doing wrong?

> I would be nice to eliminate that, but some parts of the test appear
> to pull in more of the system emulator code which forces in the main
> qapi schema
>
> n file included from ./qapi/qapi-types-block-core.h:17,
>                  from /var/home/berrange/src/virt/qemu/include/block/block-common.h:27,
>                  from /var/home/berrange/src/virt/qemu/include/block/block-global-state.h:27,
>                  from /var/home/berrange/src/virt/qemu/include/block/block.h:27,
>                  from /var/home/berrange/src/virt/qemu/include/monitor/monitor.h:4,
>                  from /var/home/berrange/src/virt/qemu/include/qapi/qmp/dispatch.h:17,
>                  from tests/test-qapi-init-commands.h:16,
>                  from tests/test-qapi-init-commands.c:15:
> ./qapi/qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
>    16 |     QAPI_FEATURE_DEPRECATED,
>       |     ^~~~~~~~~~~~~~~~~~~~~~~
> In file included from tests/include/../test-qapi-types-sub-sub-module.h:17,
>                  from tests/include/../test-qapi-commands-sub-sub-module.h:16,
>                  from tests/include/test-qapi-commands-sub-module.h:16,
>                  from tests/test-qapi-commands.h:16,
>                  from tests/test-qapi-init-commands.c:14:
> tests/include/../test-qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
>    16 |     QAPI_FEATURE_DEPRECATED,
>       |     ^~~~~~~~~~~~~~~~~~~~~~~
> tests/test-qapi-init-commands.c: In function ‘test_qmp_init_marshal’:
> tests/test-qapi-init-commands.c:64:71: error: ‘TEST_QAPI_FEATURE_DEPRECATED’ undeclared (first use in this function); did you mean ‘QAPI_FEATURE_DEPRECATED’?
>    64 |                          qmp_marshal_test_command_features1, 0, 1u << TEST_QAPI_FEATURE_DEPRECATED);
>       |                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                                                                       QAPI_FEATURE_DEPRECATED
> tests/test-qapi-init-commands.c:64:71: note: each undeclared identifier is reported only once for each function it appears in
>
>
> Maybe the test needs to be split into two ?  One test exclusively testing
> with the tests/qapi-schema/qapi-schema-test.json, and one test exclusively
> with the main QMP QAPI schema ?

Which test is it?

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Daniel P. Berrangé]

On Mon, Aug 05, 2024 at 03:11:12PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > This allows us to include multiple QAPI schemas in the same file.
> >> >
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> 
> >> I figure you had reason to simultaneously include headers generated for
> >> multiple schemas.  Do tell :)
> >
> > I didn't want to have this patch, but the unit tests do this :-(
> >
> > [2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
> > FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o 
> > cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
> > In file included from tests/test-qapi-types-sub-sub-module.h:17,
> >                  from tests/test-qapi-visit-sub-sub-module.h:17,
> >                  from tests/test-qapi-commands-sub-sub-module.c:19:
> > tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
> >    16 |     QAPI_FEATURE_DEPRECATED,
> >       |     ^~~~~~~~~~~~~~~~~~~~~~~
> > In file included from ./qapi/qapi-types-error.h:17,
> >                  from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
> >                  from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
> >                  from tests/test-qapi-commands-sub-sub-module.c:14:
> > ./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
> >    16 |     QAPI_FEATURE_DEPRECATED,
> >       |     ^~~~~~~~~~~~~~~~~~~~~~~
> > ninja: build stopped: subcommand failed.
> > make[1]: *** [Makefile:167: run-ninja] Error 1
> > make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
> > make: *** [GNUmakefile:6: build] Error 2
> 
> Compiles for me with PATCH 5/6 taken out.  What am I doing wrong?

The bit in patch 6 which generates the enum still has the prefix:

+        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")


> 
> > I would be nice to eliminate that, but some parts of the test appear
> > to pull in more of the system emulator code which forces in the main
> > qapi schema
> >
> > n file included from ./qapi/qapi-types-block-core.h:17,
> >                  from /var/home/berrange/src/virt/qemu/include/block/block-common.h:27,
> >                  from /var/home/berrange/src/virt/qemu/include/block/block-global-state.h:27,
> >                  from /var/home/berrange/src/virt/qemu/include/block/block.h:27,
> >                  from /var/home/berrange/src/virt/qemu/include/monitor/monitor.h:4,
> >                  from /var/home/berrange/src/virt/qemu/include/qapi/qmp/dispatch.h:17,
> >                  from tests/test-qapi-init-commands.h:16,
> >                  from tests/test-qapi-init-commands.c:15:
> > ./qapi/qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
> >    16 |     QAPI_FEATURE_DEPRECATED,
> >       |     ^~~~~~~~~~~~~~~~~~~~~~~
> > In file included from tests/include/../test-qapi-types-sub-sub-module.h:17,
> >                  from tests/include/../test-qapi-commands-sub-sub-module.h:16,
> >                  from tests/include/test-qapi-commands-sub-module.h:16,
> >                  from tests/test-qapi-commands.h:16,
> >                  from tests/test-qapi-init-commands.c:14:
> > tests/include/../test-qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
> >    16 |     QAPI_FEATURE_DEPRECATED,
> >       |     ^~~~~~~~~~~~~~~~~~~~~~~
> > tests/test-qapi-init-commands.c: In function ‘test_qmp_init_marshal’:
> > tests/test-qapi-init-commands.c:64:71: error: ‘TEST_QAPI_FEATURE_DEPRECATED’ undeclared (first use in this function); did you mean ‘QAPI_FEATURE_DEPRECATED’?
> >    64 |                          qmp_marshal_test_command_features1, 0, 1u << TEST_QAPI_FEATURE_DEPRECATED);
> >       |                                                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >       |                                                                       QAPI_FEATURE_DEPRECATED
> > tests/test-qapi-init-commands.c:64:71: note: each undeclared identifier is reported only once for each function it appears in
> >
> >
> > Maybe the test needs to be split into two ?  One test exclusively testing
> > with the tests/qapi-schema/qapi-schema-test.json, and one test exclusively
> > with the main QMP QAPI schema ?
> 
> Which test is it?

I'm unclear which is using this - its just failing with plain 'make' for
me.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Aug 05, 2024 at 03:11:12PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Mon, Aug 05, 2024 at 02:22:47PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > This allows us to include multiple QAPI schemas in the same file.
>> >> >
>> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> >> 
>> >> I figure you had reason to simultaneously include headers generated for
>> >> multiple schemas.  Do tell :)
>> >
>> > I didn't want to have this patch, but the unit tests do this :-(
>> >
>> > [2/37] Compiling C object tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o
>> > FAILED: tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o 
>> > cc -m64 -Itests/libtestqapi.a.p -Itests -I../tests -I. -Iqapi -Itrace -Iui -Iui/shader -Itests/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-6 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /var/home/berrange/src/virt/qemu/linux-headers -isystem linux-headers -iquote . -iquote /var/home/berrange/src/virt/qemu -iquote /var/home/berrange/src/virt/qemu/include -iquote /var/home/berrange/src/virt/qemu/host/include/x86_64 -iquote /var/home/berrange/src/virt/qemu/host/include/generic -iquote /var/home/berrange/src/virt/qemu/tcg/i386 -pthread -msse2 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -MD -MQ tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -MF tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o.d -o tests/libtestqapi.a.p/meson-generated_.._test-qapi-commands-sub-sub-module.c.o -c tests/test-qapi-commands-sub-sub-module.c
>> > In file included from tests/test-qapi-types-sub-sub-module.h:17,
>> >                  from tests/test-qapi-visit-sub-sub-module.h:17,
>> >                  from tests/test-qapi-commands-sub-sub-module.c:19:
>> > tests/test-qapi-features.h:16:5: error: redeclaration of enumerator ‘QAPI_FEATURE_DEPRECATED’
>> >    16 |     QAPI_FEATURE_DEPRECATED,
>> >       |     ^~~~~~~~~~~~~~~~~~~~~~~
>> > In file included from ./qapi/qapi-types-error.h:17,
>> >                  from /var/home/berrange/src/virt/qemu/include/qapi/error.h:275,
>> >                  from /var/home/berrange/src/virt/qemu/include/qapi/compat-policy.h:16,
>> >                  from tests/test-qapi-commands-sub-sub-module.c:14:
>> > ./qapi/qapi-features.h:16:5: note: previous definition of ‘QAPI_FEATURE_DEPRECATED’ with type ‘enum <anonymous>’
>> >    16 |     QAPI_FEATURE_DEPRECATED,
>> >       |     ^~~~~~~~~~~~~~~~~~~~~~~
>> > ninja: build stopped: subcommand failed.
>> > make[1]: *** [Makefile:167: run-ninja] Error 1
>> > make[1]: Leaving directory '/var/home/berrange/src/virt/qemu/build'
>> > make: *** [GNUmakefile:6: build] Error 2
>> 
>> Compiles for me with PATCH 5/6 taken out.  What am I doing wrong?
>
> The bit in patch 6 which generates the enum still has the prefix:
>
> +        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")

Alright, I got it to fail with the appended patch.  I'll have a closer
look.  Thanks!

[...]

diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
index 9b77be6310..1eb0c8a8ac 100644
--- a/scripts/qapi/features.py
+++ b/scripts/qapi/features.py
@@ -55,9 +55,9 @@ def visit_end(self) -> None:
 
         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(f"    {c_enum_const('QAPI_FEATURE', name)},\n")
 
-        self._genh.add("} " + c_name(self._prefix + 'QapiFeature') + ";\n")
+        self._genh.add("} " + c_name('QapiFeature') + ";\n")
 
     def _record(self, features: List[QAPISchemaFeature]):
         for f in features:

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Markus Armbruster]

It's not just tests.  QAPI-related headers have deteriorated, and pull
in too much.  I'll try to clean this up.  Thanks!

Complications due to having multiple QAPI schemas (was: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants)

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> It's not just tests.  QAPI-related headers have deteriorated, and pull
> in too much.  I'll try to clean this up.  Thanks!

While some cleanup is certainly possible and probably useful, I don't
think I can completely solve the problem that way.

Not counting tests, we have two or three QAPI schemas, depending on how
you count:

* QEMU's main schema qapi/qapi-schema.json

  This schema defines qemu-system-FOO's QMP (plus types used for QOM
  properties and such, but that's detail we can ignore here).

* The storage daemon's schema storage-daemon/qapi/qapi-schema.json

  This is a strict subset the main schema, not a schema on its own.

  qemu-storage-daemon wants to provide a subset of qemu-system-FOO's
  QMP.  We get this as follows.

  qapi/qapi-schema.json contains nothing but include directives.
  storage-daemon/qapi/qapi-schema.json contains nothing but a subset of
  the same include directives.  The exact same code gets generated for
  these modules both times.  We always use the copy generated for the
  main schema's modules, and ignore the one generated for the storage
  daemon.  Different code gets generated for command & event
  registration, and QMP introspection.  The storage daemon uses the code
  generated for its subset.  This is a bit of a hack.

* The guest agent's schema qga/qapi-schema.json

  This schema defines qemu-ga's QMP.

Each program generally uses at most one QAPI schema.  Easy.

The exception is the guest agent.  It might have initially used just its
own QAPI schema as well, but today, it also uses parts of the main
schema.  That's because the main schema has grown tentacles into common
code.

Example: qemu-ga with -m vsock-listen prefixes its -p argument with
"vsock:" and parses the result with socket_parse().  While we can
quibble about the details here, sharing vsock address syntax and parser
between guest agent and QEMU proper makes sense.  The address structure
/ abstract syntax is defined in the main schema's qapi/sockets.json.
The address parser is declared in qemu/sockets.h, and pulls in the main
schema's qapi/qapi-types-sockets.h.

Trying to limit qemu-ga to just its own schema would make no sense.  We
*want* to share things defined in a (shared) QAPI schema.

Note that while qemu-ga C code pulls in (generated) main schema stuff,
its QAPI schema so far does not.  However, reuse of utility types like
SocketAddress or OnOffAuto from the main schema might become interesting
some day.

The QAPI generator supports more than one schema per program, although
kind of half-heartedly.  There's -p to set a prefix, which is prepended
to filenames and symbols.  But there's no systematic effort to do the
latter.  We slap it to symbols whenever we run into a collision.

Example: your patch slaps it to the generated QapiFeature enum and its
members.

Example: we don't slap it to command handlers.  If two schemas both
define command 'frobnicate', we generate a prototype qmp_frobnicate()
for both, and expect the handwritten code to supply a function suitable
for both.

We can continue slapping the prefix to symbols whenever we run into a
collision, i.e. go with your patch.  Feels like the pragmatic choice to
not block your series.

But could we do better in the long run?

Back when we created the guest agent schema, QAPI schemas were
monolithic: a single schema file, generating a single qapi-types.h and
so forth.

Today, a schema can be modular, i.e. split into multiple modules, each
generating its own qapi-types-MODULE.h and so forth.

We exploit this to have a single schema both for qemu-system-FOO's QMP
and qemu-storage-daemon's QMP, albeit in a hacky way.

What if QAPI provided cleaner means for generating multiple QMPs from a
single schema?

Then we could have a single schema, and no worries about collisions.

I'm not asking you to try this.  It's just an idea I wrote up so I don't
forget it again.

Re: [PATCH 5/6] qapi: apply schema prefix to QAPI feature enum constants

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This allows us to include multiple QAPI schemas in the same file.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

This commit prepends an optional prefix to generated uses of
QAPI_FEATURE_{DEPRECATED,UNSTABLE}.

It touches neither the handwritten definition in include/qapi/util.h,
nor the handwritten uses in qapi/qapi-util.c and
qapi/qobject-output-visitor.c.

Code generated with a prefix will not compile, unless it uses no
features.  No biggie, because:

The next commit replaces the handwritten definition (no prefix) by a
generated one (with optional prefix).

Now the handwritten code compiles only because it includes an enum
definition generated without a prefix, namely the main schema's.

Note that the handwritten code continues to work even when you use it
together with some other schema, but only because all the generated
enums assign the same numeric value to the enumeration constants
_DEPRECATED and _UNSTABLE.

While that seems fairly unlikely to break accidentally, it still feels
unnecessarily dirty.

What about generating an enum like this:

    typedef enum {
        QAPI_FEATURE_DEPRECATED = QAPI_DEPRECATED,
        QAPI_FEATURE_UNSTABLE = QAPI_UNSTABLE,
        ...
    } QapiFeature;

where QAPI_DEPRECATED and QAPI_UNSTABLE are defined in qapi/util.h, like
they are before this series.

We can discuss renaming them to QAPI_SPECIAL_FEATURE_DEPRECATED and
_UNSTABLE if you like.

Code referring to special features, i.e. all references before this
series, use the definitions from qapi/util.h.

Code referring to arbitrary, possibly non-special features, i.e. the
references new in this series, use the generated definitions from
qapi/qapi-features.h.

Thoughts?

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

[Markus Armbruster]

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