* [PATCH v2 1/4] qapi: cope with feature names containing a '-'
2024-10-18 10:17 [PATCH v2 0/4] qapi: generalize special features Daniel P. Berrangé
@ 2024-10-18 10:17 ` Daniel P. Berrangé
2024-10-18 10:17 ` [PATCH v2 2/4] qapi: change 'unsigned special_features' to 'uint64_t features' Daniel P. Berrangé
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2024-10-18 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Michael Roth, Markus Armbruster,
Marc-André Lureau
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 6a8abe0041..c53ca72950 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_special_features(features: Sequence[QAPISchemaFeature]) -> str:
- special_features = [f"1u << QAPI_{feat.name.upper()}"
+ special_features = [f"1u << {c_enum_const('qapi', feat.name)}"
for feat in features if feat.is_special()]
return ' | '.join(special_features) or '0'
--
2.46.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/4] qapi: change 'unsigned special_features' to 'uint64_t features'
2024-10-18 10:17 [PATCH v2 0/4] qapi: generalize special features Daniel P. Berrangé
2024-10-18 10:17 ` [PATCH v2 1/4] qapi: cope with feature names containing a '-' Daniel P. Berrangé
@ 2024-10-18 10:17 ` Daniel P. Berrangé
2024-10-21 5:50 ` Philippe Mathieu-Daudé
2024-10-18 10:17 ` [PATCH v2 3/4] qapi: rename 'special_features' to 'features' Daniel P. Berrangé
2024-10-18 10:17 ` [PATCH v2 4/4] qapi: expose all schema features to code Daniel P. Berrangé
3 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2024-10-18 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Michael Roth, Markus Armbruster,
Marc-André Lureau
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 | 2 +-
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, 34 insertions(+), 34 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..29bc4eb865 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -18,7 +18,7 @@ typedef enum {
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.46.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] qapi: change 'unsigned special_features' to 'uint64_t features'
2024-10-18 10:17 ` [PATCH v2 2/4] qapi: change 'unsigned special_features' to 'uint64_t features' Daniel P. Berrangé
@ 2024-10-21 5:50 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-10-21 5:50 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Paolo Bonzini, Michael Roth, Markus Armbruster,
Marc-André Lureau
On 18/10/24 07:17, Daniel P. Berrangé wrote:
> 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 | 2 +-
> 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, 34 insertions(+), 34 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] qapi: rename 'special_features' to 'features'
2024-10-18 10:17 [PATCH v2 0/4] qapi: generalize special features Daniel P. Berrangé
2024-10-18 10:17 ` [PATCH v2 1/4] qapi: cope with feature names containing a '-' Daniel P. Berrangé
2024-10-18 10:17 ` [PATCH v2 2/4] qapi: change 'unsigned special_features' to 'uint64_t features' Daniel P. Berrangé
@ 2024-10-18 10:17 ` Daniel P. Berrangé
2024-10-21 5:50 ` Philippe Mathieu-Daudé
2024-11-13 13:20 ` Markus Armbruster
2024-10-18 10:17 ` [PATCH v2 4/4] qapi: expose all schema features to code Daniel P. Berrangé
3 siblings, 2 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2024-10-18 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Michael Roth, Markus Armbruster,
Marc-André Lureau
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 | 6 +++---
scripts/qapi/types.py | 10 +++++-----
scripts/qapi/visit.py | 14 +++++++-------
4 files changed, 17 insertions(+), 17 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 c53ca72950..aba1a982f6 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -41,10 +41,10 @@
from .source import QAPISourceInfo
-def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
- special_features = [f"1u << {c_enum_const('qapi', feat.name)}"
+def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
+ features = [f"1u << {c_enum_const('qapi', feat.name)}"
for feat in features if feat.is_special()]
- return ' | '.join(special_features) or '0'
+ 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.46.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/4] qapi: rename 'special_features' to 'features'
2024-10-18 10:17 ` [PATCH v2 3/4] qapi: rename 'special_features' to 'features' Daniel P. Berrangé
@ 2024-10-21 5:50 ` Philippe Mathieu-Daudé
2024-11-13 13:20 ` Markus Armbruster
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-10-21 5:50 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Paolo Bonzini, Michael Roth, Markus Armbruster,
Marc-André Lureau
On 18/10/24 07:17, Daniel P. Berrangé wrote:
> 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 | 6 +++---
> scripts/qapi/types.py | 10 +++++-----
> scripts/qapi/visit.py | 14 +++++++-------
> 4 files changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] qapi: rename 'special_features' to 'features'
2024-10-18 10:17 ` [PATCH v2 3/4] qapi: rename 'special_features' to 'features' Daniel P. Berrangé
2024-10-21 5:50 ` Philippe Mathieu-Daudé
@ 2024-11-13 13:20 ` Markus Armbruster
1 sibling, 0 replies; 12+ messages in thread
From: Markus Armbruster @ 2024-11-13 13:20 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Michael Roth, Marc-André Lureau
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>
> ---
> scripts/qapi/commands.py | 4 ++--
> scripts/qapi/gen.py | 6 +++---
> scripts/qapi/types.py | 10 +++++-----
> scripts/qapi/visit.py | 14 +++++++-------
> 4 files changed, 17 insertions(+), 17 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 c53ca72950..aba1a982f6 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -41,10 +41,10 @@
> from .source import QAPISourceInfo
>
>
> -def gen_special_features(features: Sequence[QAPISchemaFeature]) -> str:
> - special_features = [f"1u << {c_enum_const('qapi', feat.name)}"
> +def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
> + features = [f"1u << {c_enum_const('qapi', feat.name)}"
> for feat in features if feat.is_special()]
pycodestyle gripes
scripts/qapi/gen.py:46:25: E127 continuation line over-indented for visual indent
More seriously, mypy gripes
scripts/qapi/gen.py:45: error: List comprehension has incompatible type List[str]; expected List[QAPISchemaFeature] [misc]
here, and ...
> - return ' | '.join(special_features) or '0'
> + return ' | '.join(features) or '0'
scripts/qapi/gen.py:47: error: Argument 1 to "join" of "str" has incompatible type "List[QAPISchemaFeature]"; expected "Iterable[str]" [arg-type]
This is due to your assignment to parameter @feature. Use a new
variable instead.
>
>
> class QAPIGen:
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] qapi: expose all schema features to code
2024-10-18 10:17 [PATCH v2 0/4] qapi: generalize special features Daniel P. Berrangé
` (2 preceding siblings ...)
2024-10-18 10:17 ` [PATCH v2 3/4] qapi: rename 'special_features' to 'features' Daniel P. Berrangé
@ 2024-10-18 10:17 ` Daniel P. Berrangé
2024-11-14 12:48 ` Markus Armbruster
3 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2024-10-18 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Michael Roth, Markus Armbruster,
Marc-André Lureau
This replaces use of the constants from the QapiSpecialFeatures
enum, with constants from the auto-generate QapiFeatures enum
in qapi-features.h
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
retains compatibility with common code that references the features
via the QapiSpecialFeatures constants.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
meson.build | 1 +
scripts/qapi/commands.py | 1 +
scripts/qapi/features.py | 62 ++++++++++++++++++++++++++++++++++++++++
scripts/qapi/gen.py | 4 +--
scripts/qapi/main.py | 2 ++
scripts/qapi/schema.py | 19 +++++++++++-
scripts/qapi/types.py | 6 ++--
scripts/qapi/visit.py | 3 +-
8 files changed, 92 insertions(+), 6 deletions(-)
create mode 100644 scripts/qapi/features.py
diff --git a/meson.build b/meson.build
index d26690ce20..b9d58be66f 100644
--- a/meson.build
+++ b/meson.build
@@ -3332,6 +3332,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/commands.py b/scripts/qapi/commands.py
index d629d2d97e..bf88bfc442 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
#include "qemu/osdep.h"
#include "%(prefix)sqapi-commands.h"
#include "%(prefix)sqapi-init-commands.h"
+#include "%(prefix)sqapi-features.h"
void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
{
diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
new file mode 100644
index 0000000000..dc10c7cea0
--- /dev/null
+++ b/scripts/qapi/features.py
@@ -0,0 +1,62 @@
+"""
+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,
+ QAPISchemaFeature,
+)
+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_begin(self, schema: QAPISchema):
+ self.features = schema._feature_dict
+
+ def visit_end(self) -> None:
+ features = [
+ self.features[f]
+ for f in QAPISchemaFeature.SPECIAL_NAMES
+ ]
+
+ features.extend(
+ sorted(
+ filter(lambda f: not f.is_special(),
+ self.features.values()),
+ key=lambda f: f.name)
+ )
+
+ self._genh.add("typedef enum {\n")
+ for f in features:
+ self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
+ if f.name in QAPISchemaFeature.SPECIAL_NAMES:
+ self._genh.add(f" = {c_enum_const('qapi', f.name)},\n" )
+ else:
+ self._genh.add(",\n")
+
+ self._genh.add("} " + c_name('QapiFeature') + ";\n")
+
+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/gen.py b/scripts/qapi/gen.py
index aba1a982f6..6ef603941c 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -42,8 +42,8 @@
def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
- features = [f"1u << {c_enum_const('qapi', feat.name)}"
- for feat in features if feat.is_special()]
+ features = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
+ for feat in features]
return ' | '.join(features) or '0'
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 e97c978d38..5e14b1829b 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):
@@ -1138,6 +1141,11 @@ def __init__(self, fname: str):
self._entity_list: List[QAPISchemaEntity] = []
self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
+ self._feature_dict: Dict[str, QAPISchemaFeature] = {}
+
+ for f in QAPISchemaFeature.SPECIAL_NAMES:
+ self._feature_dict[f] = QAPISchemaFeature(f, "special feature")
+
self._schema_dir = os.path.dirname(fname)
self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
self._make_module(fname)
@@ -1258,6 +1266,15 @@ def _make_features(
) -> List[QAPISchemaFeature]:
if features is None:
return []
+
+ for f in features:
+ feat = QAPISchemaFeature(f['name'], info)
+ if feat.name not in self._feature_dict:
+ if len(self._feature_dict) == 64:
+ raise Exception("Maximum of 64 schema features is permitted")
+
+ self._feature_dict[feat.name] = feat
+
return [QAPISchemaFeature(f['name'], info,
QAPISchemaIfCond(f.get('if')))
for f in features]
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index ade6b7a3d7..7618d3eb6f 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -308,11 +308,13 @@ def _begin_user_module(self, name: str) -> None:
#include "qapi/dealloc-visitor.h"
#include "%(types)s.h"
#include "%(visit)s.h"
+#include "%(prefix)sqapi-features.h"
''',
- types=types, visit=visit))
+ types=types, visit=visit, prefix=self._prefix))
self._genh.preamble_add(mcgen('''
#include "qapi/qapi-builtin-types.h"
-'''))
+''',
+ prefix=self._prefix))
def visit_begin(self, schema: QAPISchema) -> None:
# gen_object() is recursive, ensure it doesn't visit the empty type
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 8dbf4ef1c3..2d678c281d 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -360,8 +360,9 @@ def _begin_user_module(self, name: str) -> None:
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "%(visit)s.h"
+#include "%(prefix)sqapi-features.h"
''',
- visit=visit))
+ visit=visit, prefix=self._prefix))
self._genh.preamble_add(mcgen('''
#include "qapi/qapi-builtin-visit.h"
#include "%(types)s.h"
--
2.46.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] qapi: expose all schema features to code
2024-10-18 10:17 ` [PATCH v2 4/4] qapi: expose all schema features to code Daniel P. Berrangé
@ 2024-11-14 12:48 ` Markus Armbruster
2024-11-14 13:25 ` Daniel P. Berrangé
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2024-11-14 12:48 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Michael Roth, Markus Armbruster, Marc-André Lureau
Daniel P. Berrangé <berrange@redhat.com> writes:
> This replaces use of the constants from the QapiSpecialFeatures
> enum, with constants from the auto-generate QapiFeatures enum
> in qapi-features.h
>
> 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
> retains compatibility with common code that references the features
> via the QapiSpecialFeatures constants.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> meson.build | 1 +
> scripts/qapi/commands.py | 1 +
> scripts/qapi/features.py | 62 ++++++++++++++++++++++++++++++++++++++++
> scripts/qapi/gen.py | 4 +--
> scripts/qapi/main.py | 2 ++
> scripts/qapi/schema.py | 19 +++++++++++-
> scripts/qapi/types.py | 6 ++--
> scripts/qapi/visit.py | 3 +-
> 8 files changed, 92 insertions(+), 6 deletions(-)
> create mode 100644 scripts/qapi/features.py
>
> diff --git a/meson.build b/meson.build
> index d26690ce20..b9d58be66f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3332,6 +3332,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/commands.py b/scripts/qapi/commands.py
> index d629d2d97e..bf88bfc442 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> #include "qemu/osdep.h"
> #include "%(prefix)sqapi-commands.h"
> #include "%(prefix)sqapi-init-commands.h"
> +#include "%(prefix)sqapi-features.h"
>
> void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
> {
> diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
> new file mode 100644
> index 0000000000..dc10c7cea0
> --- /dev/null
> +++ b/scripts/qapi/features.py
> @@ -0,0 +1,62 @@
> +"""
> +QAPI types generator
QAPI features 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
pylint gripes
scripts/qapi/features.py:10:0: W0611: Unused List imported from typing (unused-
import)
scripts/qapi/features.py:10:0: W0611: Unused Optional imported from typing (unused-import)
> +
> +from .common import c_enum_const, mcgen, c_name
scripts/qapi/features.py:12:0: W0611: Unused mcgen imported from common (unused-import)
> +from .gen import QAPISchemaMonolithicCVisitor
> +from .schema import (
> + QAPISchema,
> + QAPISchemaFeature,
> +)
> +from .source import QAPISourceInfo
scripts/qapi/features.py:18:0: W0611: Unused QAPISourceInfo imported from source (unused-import)
> +
> +
> +class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
> +
> + def __init__(self, prefix: str):
> + super().__init__(
> + prefix, 'qapi-features',
> + ' * Schema-defined QAPI features',
> + __doc__)
> +
> + self.features = {}
mypy gripes
scripts/qapi/features.py:29: error: Need type annotation for "features" (hint: "features: Dict[<type>, <type>] = ...") [var-annotated]
Elsewhere, we avoid rummaging in QAPISchema's innards by defining
suitable visit. If that's too much trouble for you now, I can take this
as is an clean up on top.
> +
> + def visit_begin(self, schema: QAPISchema):
mypy gripes
scripts/qapi/features.py:31: error: Function is missing a return type annotation [no-untyped-def]
> + self.features = schema._feature_dict
pylint gripes
scripts/qapi/features.py:32:24: W0212: Access to a protected member _feature_dict of a client class (protected-access)
> +
> + def visit_end(self) -> None:
> + features = [
> + self.features[f]
> + for f in QAPISchemaFeature.SPECIAL_NAMES
> + ]
> +
> + features.extend(
> + sorted(
> + filter(lambda f: not f.is_special(),
> + self.features.values()),
> + key=lambda f: f.name)
> + )
@features is schema._feature_dict.values() sorted by name in a certain
way, namely first the .SPECIAL_NAMES in order, then all the others in
alphabetical order.
Why you do this is not immediately obvious. I guess it's to make the
generated enum a compatible extension of enum QapiSpecialFeature. That
one exists for use by schema-independent support code such
compat_policy_input_ok() and qobject_output_policy_skip().
I further guess you sort the non-special features just to make the
generated code easier for humans to navigate.
Correct?
> +
> + self._genh.add("typedef enum {\n")
> + for f in features:
> + self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
> + if f.name in QAPISchemaFeature.SPECIAL_NAMES:
> + self._genh.add(f" = {c_enum_const('qapi', f.name)},\n" )
pycodestyle gripes
scripts/qapi/features.py:51:71: E202 whitespace before ')'
> + else:
> + self._genh.add(",\n")
> +
> + self._genh.add("} " + c_name('QapiFeature') + ";\n")
> +
pycodestyle gripes
scripts/qapi/features.py:57:1: E302 expected 2 blank lines, found 1
This part generates a C enum. It's similar to gen_enum() from types.py,
except we work with a list of QAPISchemaFeature here, and a list of
QAPISchemaEnumMember there.
To reuse gen_enum() here, we'd have to make up a member list, like we do
in events.py for enum QAPIEvent.
> +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/gen.py b/scripts/qapi/gen.py
> index aba1a982f6..6ef603941c 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -42,8 +42,8 @@
>
>
> def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
> - features = [f"1u << {c_enum_const('qapi', feat.name)}"
> - for feat in features if feat.is_special()]
> + features = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
> + for feat in features]
> return ' | '.join(features) or '0'
>
>
> 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 e97c978d38..5e14b1829b 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):
> @@ -1138,6 +1141,11 @@ def __init__(self, fname: str):
> self._entity_list: List[QAPISchemaEntity] = []
> self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
> self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
> + self._feature_dict: Dict[str, QAPISchemaFeature] = {}
> +
> + for f in QAPISchemaFeature.SPECIAL_NAMES:
> + self._feature_dict[f] = QAPISchemaFeature(f, "special feature")
mypy gripes
scripts/qapi/schema.py:1147: error: Argument 2 to "QAPISchemaFeature" has incompatible type "str"; expected "Optional[QAPISourceInfo]" [arg-type]
We commonly use None as info value for built-in stuff, and that's why
it's Optional[QAPISourceInfo], not just QAPISourceInfo.
But do we really need to make up some QAPISchemaFeature? Hmm. The
appended patch dumbs down ._feature_dict to a set.
> +
> self._schema_dir = os.path.dirname(fname)
> self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
> self._make_module(fname)
> @@ -1258,6 +1266,15 @@ def _make_features(
> ) -> List[QAPISchemaFeature]:
> if features is None:
> return []
> +
> + for f in features:
> + feat = QAPISchemaFeature(f['name'], info)
> + if feat.name not in self._feature_dict:
> + if len(self._feature_dict) == 64:
> + raise Exception("Maximum of 64 schema features is permitted")
The limit is an implementation restriction. Okay, we can lift it when
it bites us.
However, the reporting is less than nice:
$ python scripts/qapi-gen.py -o $$ tests/qapi-schema/features-too-many.json
Traceback (most recent call last):
File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
sys.exit(main.main())
^^^^^^^^^^^
File "/work/armbru/qemu/scripts/qapi/main.py", line 96, in main
generate(args.schema,
File "/work/armbru/qemu/scripts/qapi/main.py", line 51, in generate
schema = QAPISchema(schema_file)
^^^^^^^^^^^^^^^^^^^^^^^
File "/work/armbru/qemu/scripts/qapi/schema.py", line 1155, in __init__
self._def_exprs(exprs)
File "/work/armbru/qemu/scripts/qapi/schema.py", line 1482, in _def_exprs
self._def_struct_type(expr)
File "/work/armbru/qemu/scripts/qapi/schema.py", line 1377, in _def_struct_type
features = self._make_features(expr.get('features'), info)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/work/armbru/qemu/scripts/qapi/schema.py", line 1274, in _make_features
raise Exception("Maximum of 64 schema features is permitted")
Exception: Maximum of 64 schema features is permitted
> +
> + self._feature_dict[feat.name] = feat
> +
> return [QAPISchemaFeature(f['name'], info,
> QAPISchemaIfCond(f.get('if')))
> for f in features]
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index ade6b7a3d7..7618d3eb6f 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -308,11 +308,13 @@ def _begin_user_module(self, name: str) -> None:
> #include "qapi/dealloc-visitor.h"
> #include "%(types)s.h"
> #include "%(visit)s.h"
> +#include "%(prefix)sqapi-features.h"
> ''',
> - types=types, visit=visit))
> + types=types, visit=visit, prefix=self._prefix))
Long line, easy to break:
types=types, visit=visit,
prefix=self._prefix))
> self._genh.preamble_add(mcgen('''
> #include "qapi/qapi-builtin-types.h"
> -'''))
> +''',
> + prefix=self._prefix))
>
> def visit_begin(self, schema: QAPISchema) -> None:
> # gen_object() is recursive, ensure it doesn't visit the empty type
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 8dbf4ef1c3..2d678c281d 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -360,8 +360,9 @@ def _begin_user_module(self, name: str) -> None:
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "%(visit)s.h"
> +#include "%(prefix)sqapi-features.h"
> ''',
> - visit=visit))
> + visit=visit, prefix=self._prefix))
> self._genh.preamble_add(mcgen('''
> #include "qapi/qapi-builtin-visit.h"
> #include "%(types)s.h"
diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
index dc10c7cea0..3662c1e568 100644
--- a/scripts/qapi/features.py
+++ b/scripts/qapi/features.py
@@ -7,7 +7,7 @@
# See the COPYING file in the top-level directory.
"""
-from typing import List, Optional
+from typing import List, Optional, Set
from .common import c_enum_const, mcgen, c_name
from .gen import QAPISchemaMonolithicCVisitor
@@ -26,29 +26,25 @@ def __init__(self, prefix: str):
' * Schema-defined QAPI features',
__doc__)
- self.features = {}
+ self.features: Set['str']
- def visit_begin(self, schema: QAPISchema):
- self.features = schema._feature_dict
+ def visit_begin(self, schema: QAPISchema) -> None:
+ self.features = schema._features
def visit_end(self) -> None:
- features = [
- self.features[f]
- for f in QAPISchemaFeature.SPECIAL_NAMES
- ]
+ features = QAPISchemaFeature.SPECIAL_NAMES.copy()
features.extend(
sorted(
- filter(lambda f: not f.is_special(),
- self.features.values()),
- key=lambda f: f.name)
+ filter(lambda f: f not in QAPISchemaFeature.SPECIAL_NAMES,
+ self.features))
)
self._genh.add("typedef enum {\n")
for f in features:
- self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
- if f.name in QAPISchemaFeature.SPECIAL_NAMES:
- self._genh.add(f" = {c_enum_const('qapi', f.name)},\n" )
+ self._genh.add(f" {c_enum_const('qapi_feature', f)}")
+ if f in QAPISchemaFeature.SPECIAL_NAMES:
+ self._genh.add(f" = {c_enum_const('qapi', f)},\n" )
else:
self._genh.add(",\n")
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 6ef603941c..3fb84d36c2 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -42,9 +42,9 @@
def gen_features(features: Sequence[QAPISchemaFeature]) -> str:
- features = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
+ feats = [f"1u << {c_enum_const('qapi_feature', feat.name)}"
for feat in features]
- return ' | '.join(features) or '0'
+ return ' | '.join(feats) or '0'
class QAPIGen:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 5e14b1829b..dd71cbc8b7 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -28,6 +28,7 @@
Dict,
List,
Optional,
+ Set,
Union,
cast,
)
@@ -1141,10 +1142,7 @@ def __init__(self, fname: str):
self._entity_list: List[QAPISchemaEntity] = []
self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
- self._feature_dict: Dict[str, QAPISchemaFeature] = {}
-
- for f in QAPISchemaFeature.SPECIAL_NAMES:
- self._feature_dict[f] = QAPISchemaFeature(f, "special feature")
+ self._features: Set[str] = set(QAPISchemaFeature.SPECIAL_NAMES)
self._schema_dir = os.path.dirname(fname)
self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
@@ -1269,11 +1267,11 @@ def _make_features(
for f in features:
feat = QAPISchemaFeature(f['name'], info)
- if feat.name not in self._feature_dict:
- if len(self._feature_dict) == 64:
+ if feat.name not in self._features:
+ if len(self._features) == 64:
raise Exception("Maximum of 64 schema features is permitted")
- self._feature_dict[feat.name] = feat
+ self._features.add(feat.name)
return [QAPISchemaFeature(f['name'], info,
QAPISchemaIfCond(f.get('if')))
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] qapi: expose all schema features to code
2024-11-14 12:48 ` Markus Armbruster
@ 2024-11-14 13:25 ` Daniel P. Berrangé
2024-11-15 7:47 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2024-11-14 13:25 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Michael Roth, Marc-André Lureau
On Thu, Nov 14, 2024 at 01:48:28PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > This replaces use of the constants from the QapiSpecialFeatures
> > enum, with constants from the auto-generate QapiFeatures enum
> > in qapi-features.h
> >
> > 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
> > retains compatibility with common code that references the features
> > via the QapiSpecialFeatures constants.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > meson.build | 1 +
> > scripts/qapi/commands.py | 1 +
> > scripts/qapi/features.py | 62 ++++++++++++++++++++++++++++++++++++++++
> > scripts/qapi/gen.py | 4 +--
> > scripts/qapi/main.py | 2 ++
> > scripts/qapi/schema.py | 19 +++++++++++-
> > scripts/qapi/types.py | 6 ++--
> > scripts/qapi/visit.py | 3 +-
> > 8 files changed, 92 insertions(+), 6 deletions(-)
> > create mode 100644 scripts/qapi/features.py
> >
> > diff --git a/meson.build b/meson.build
> > index d26690ce20..b9d58be66f 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -3332,6 +3332,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/commands.py b/scripts/qapi/commands.py
> > index d629d2d97e..bf88bfc442 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
> > #include "qemu/osdep.h"
> > #include "%(prefix)sqapi-commands.h"
> > #include "%(prefix)sqapi-init-commands.h"
> > +#include "%(prefix)sqapi-features.h"
> >
> > void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
> > {
> > diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
> > new file mode 100644
> > index 0000000000..dc10c7cea0
> > --- /dev/null
> > +++ b/scripts/qapi/features.py
> > @@ -0,0 +1,62 @@
> > +"""
> > +QAPI types generator
>
> QAPI features 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
>
> pylint gripes
Sigh, I really wish we had pylint/mypy/pycodestyle being run as
part of 'make check' by default. I don't like making the mistake
of sending patches which fail extra non-default checks maintainers
need compliance with.
>
> scripts/qapi/features.py:10:0: W0611: Unused List imported from typing (unused-
> import)
> scripts/qapi/features.py:10:0: W0611: Unused Optional imported from typing (unused-import)
>
> > +
> > +from .common import c_enum_const, mcgen, c_name
>
> scripts/qapi/features.py:12:0: W0611: Unused mcgen imported from common (unused-import)
>
> > +from .gen import QAPISchemaMonolithicCVisitor
> > +from .schema import (
> > + QAPISchema,
> > + QAPISchemaFeature,
> > +)
> > +from .source import QAPISourceInfo
>
> scripts/qapi/features.py:18:0: W0611: Unused QAPISourceInfo imported from source (unused-import)
>
> > +
> > +
> > +class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
> > +
> > + def __init__(self, prefix: str):
> > + super().__init__(
> > + prefix, 'qapi-features',
> > + ' * Schema-defined QAPI features',
> > + __doc__)
> > +
> > + self.features = {}
>
> mypy gripes
>
> scripts/qapi/features.py:29: error: Need type annotation for "features" (hint: "features: Dict[<type>, <type>] = ...") [var-annotated]
>
> Elsewhere, we avoid rummaging in QAPISchema's innards by defining
> suitable visit. If that's too much trouble for you now, I can take this
> as is an clean up on top.
>
> > +
> > + def visit_begin(self, schema: QAPISchema):
>
> mypy gripes
>
> scripts/qapi/features.py:31: error: Function is missing a return type annotation [no-untyped-def]
>
> > + self.features = schema._feature_dict
>
> pylint gripes
>
> scripts/qapi/features.py:32:24: W0212: Access to a protected member _feature_dict of a client class (protected-access)
>
> > +
> > + def visit_end(self) -> None:
> > + features = [
> > + self.features[f]
> > + for f in QAPISchemaFeature.SPECIAL_NAMES
> > + ]
> > +
> > + features.extend(
> > + sorted(
> > + filter(lambda f: not f.is_special(),
> > + self.features.values()),
> > + key=lambda f: f.name)
> > + )
>
> @features is schema._feature_dict.values() sorted by name in a certain
> way, namely first the .SPECIAL_NAMES in order, then all the others in
> alphabetical order.
>
> Why you do this is not immediately obvious. I guess it's to make the
> generated enum a compatible extension of enum QapiSpecialFeature. That
> one exists for use by schema-independent support code such
> compat_policy_input_ok() and qobject_output_policy_skip().
Yes, I wanted the overlapping enums vaules to match.
> I further guess you sort the non-special features just to make the
> generated code easier for humans to navigate.
>
> Correct?
The remaining sort was just to give a predictable stable output,
should QAPI usage of features be reordered.
> > +
> > + self._genh.add("typedef enum {\n")
> > + for f in features:
> > + self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
> > + if f.name in QAPISchemaFeature.SPECIAL_NAMES:
> > + self._genh.add(f" = {c_enum_const('qapi', f.name)},\n" )
>
> pycodestyle gripes
>
> scripts/qapi/features.py:51:71: E202 whitespace before ')'
>
> > + else:
> > + self._genh.add(",\n")
> > +
> > + self._genh.add("} " + c_name('QapiFeature') + ";\n")
> > +
>
> pycodestyle gripes
>
> scripts/qapi/features.py:57:1: E302 expected 2 blank lines, found 1
>
> This part generates a C enum. It's similar to gen_enum() from types.py,
> except we work with a list of QAPISchemaFeature here, and a list of
> QAPISchemaEnumMember there.
>
> To reuse gen_enum() here, we'd have to make up a member list, like we do
> in events.py for enum QAPIEvent.
I'll have a look at that.
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index e97c978d38..5e14b1829b 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):
> > @@ -1138,6 +1141,11 @@ def __init__(self, fname: str):
> > self._entity_list: List[QAPISchemaEntity] = []
> > self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
> > self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
> > + self._feature_dict: Dict[str, QAPISchemaFeature] = {}
> > +
> > + for f in QAPISchemaFeature.SPECIAL_NAMES:
> > + self._feature_dict[f] = QAPISchemaFeature(f, "special feature")
>
> mypy gripes
>
> scripts/qapi/schema.py:1147: error: Argument 2 to "QAPISchemaFeature" has incompatible type "str"; expected "Optional[QAPISourceInfo]" [arg-type]
>
> We commonly use None as info value for built-in stuff, and that's why
> it's Optional[QAPISourceInfo], not just QAPISourceInfo.
Yeah, not sure what I was thinking here, looking again I
should have passed "None"
> But do we really need to make up some QAPISchemaFeature? Hmm. The
> appended patch dumbs down ._feature_dict to a set.
I was following the same pattern as self._entity_dict and
self._module_dict, rather than dumbing down to the bare
minimum needed by my current use case. I don't mind which
strategy we take.
>
> > +
> > self._schema_dir = os.path.dirname(fname)
> > self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
> > self._make_module(fname)
> > @@ -1258,6 +1266,15 @@ def _make_features(
> > ) -> List[QAPISchemaFeature]:
> > if features is None:
> > return []
> > +
> > + for f in features:
> > + feat = QAPISchemaFeature(f['name'], info)
> > + if feat.name not in self._feature_dict:
> > + if len(self._feature_dict) == 64:
> > + raise Exception("Maximum of 64 schema features is permitted")
>
> The limit is an implementation restriction. Okay, we can lift it when
> it bites us.
>
> However, the reporting is less than nice:
>
> $ python scripts/qapi-gen.py -o $$ tests/qapi-schema/features-too-many.json
> Traceback (most recent call last):
> File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
> sys.exit(main.main())
> ^^^^^^^^^^^
> File "/work/armbru/qemu/scripts/qapi/main.py", line 96, in main
> generate(args.schema,
> File "/work/armbru/qemu/scripts/qapi/main.py", line 51, in generate
> schema = QAPISchema(schema_file)
> ^^^^^^^^^^^^^^^^^^^^^^^
> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1155, in __init__
> self._def_exprs(exprs)
> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1482, in _def_exprs
> self._def_struct_type(expr)
> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1377, in _def_struct_type
> features = self._make_features(expr.get('features'), info)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1274, in _make_features
> raise Exception("Maximum of 64 schema features is permitted")
> Exception: Maximum of 64 schema features is permitted
Is there any better way to approach this error reporting ?
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] qapi: expose all schema features to code
2024-11-14 13:25 ` Daniel P. Berrangé
@ 2024-11-15 7:47 ` Markus Armbruster
2024-11-15 18:30 ` Daniel P. Berrangé
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2024-11-15 7:47 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Michael Roth, Marc-André Lureau
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Thu, Nov 14, 2024 at 01:48:28PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > This replaces use of the constants from the QapiSpecialFeatures
>> > enum, with constants from the auto-generate QapiFeatures enum
>> > in qapi-features.h
>> >
>> > 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
>> > retains compatibility with common code that references the features
>> > via the QapiSpecialFeatures constants.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> > meson.build | 1 +
>> > scripts/qapi/commands.py | 1 +
>> > scripts/qapi/features.py | 62 ++++++++++++++++++++++++++++++++++++++++
>> > scripts/qapi/gen.py | 4 +--
>> > scripts/qapi/main.py | 2 ++
>> > scripts/qapi/schema.py | 19 +++++++++++-
>> > scripts/qapi/types.py | 6 ++--
>> > scripts/qapi/visit.py | 3 +-
>> > 8 files changed, 92 insertions(+), 6 deletions(-)
>> > create mode 100644 scripts/qapi/features.py
>> >
>> > diff --git a/meson.build b/meson.build
>> > index d26690ce20..b9d58be66f 100644
>> > --- a/meson.build
>> > +++ b/meson.build
>> > @@ -3332,6 +3332,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/commands.py b/scripts/qapi/commands.py
>> > index d629d2d97e..bf88bfc442 100644
>> > --- a/scripts/qapi/commands.py
>> > +++ b/scripts/qapi/commands.py
>> > @@ -355,6 +355,7 @@ def visit_begin(self, schema: QAPISchema) -> None:
>> > #include "qemu/osdep.h"
>> > #include "%(prefix)sqapi-commands.h"
>> > #include "%(prefix)sqapi-init-commands.h"
>> > +#include "%(prefix)sqapi-features.h"
>> >
>> > void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds)
>> > {
>> > diff --git a/scripts/qapi/features.py b/scripts/qapi/features.py
>> > new file mode 100644
>> > index 0000000000..dc10c7cea0
>> > --- /dev/null
>> > +++ b/scripts/qapi/features.py
>> > @@ -0,0 +1,62 @@
>> > +"""
>> > +QAPI types generator
>>
>> QAPI features 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
>>
>> pylint gripes
>
> Sigh, I really wish we had pylint/mypy/pycodestyle being run as
> part of 'make check' by default. I don't like making the mistake
> of sending patches which fail extra non-default checks maintainers
> need compliance with.
We've made some progress towards running these checkers, but we're not
there, yet.
>> scripts/qapi/features.py:10:0: W0611: Unused List imported from typing (unused-
>> import)
>> scripts/qapi/features.py:10:0: W0611: Unused Optional imported from typing (unused-import)
>>
>> > +
>> > +from .common import c_enum_const, mcgen, c_name
>>
>> scripts/qapi/features.py:12:0: W0611: Unused mcgen imported from common (unused-import)
>>
>> > +from .gen import QAPISchemaMonolithicCVisitor
>> > +from .schema import (
>> > + QAPISchema,
>> > + QAPISchemaFeature,
>> > +)
>> > +from .source import QAPISourceInfo
>>
>> scripts/qapi/features.py:18:0: W0611: Unused QAPISourceInfo imported from source (unused-import)
>>
>> > +
>> > +
>> > +class QAPISchemaGenFeatureVisitor(QAPISchemaMonolithicCVisitor):
>> > +
>> > + def __init__(self, prefix: str):
>> > + super().__init__(
>> > + prefix, 'qapi-features',
>> > + ' * Schema-defined QAPI features',
>> > + __doc__)
>> > +
>> > + self.features = {}
>>
>> mypy gripes
>>
>> scripts/qapi/features.py:29: error: Need type annotation for "features" (hint: "features: Dict[<type>, <type>] = ...") [var-annotated]
>>
>> Elsewhere, we avoid rummaging in QAPISchema's innards by defining
>> suitable visit. If that's too much trouble for you now, I can take this
>> as is an clean up on top.
>>
>> > +
>> > + def visit_begin(self, schema: QAPISchema):
>>
>> mypy gripes
>>
>> scripts/qapi/features.py:31: error: Function is missing a return type annotation [no-untyped-def]
>>
>> > + self.features = schema._feature_dict
>>
>> pylint gripes
>>
>> scripts/qapi/features.py:32:24: W0212: Access to a protected member _feature_dict of a client class (protected-access)
>>
>> > +
>> > + def visit_end(self) -> None:
>> > + features = [
>> > + self.features[f]
>> > + for f in QAPISchemaFeature.SPECIAL_NAMES
>> > + ]
>> > +
>> > + features.extend(
>> > + sorted(
>> > + filter(lambda f: not f.is_special(),
>> > + self.features.values()),
>> > + key=lambda f: f.name)
>> > + )
>>
>> @features is schema._feature_dict.values() sorted by name in a certain
>> way, namely first the .SPECIAL_NAMES in order, then all the others in
>> alphabetical order.
>>
>> Why you do this is not immediately obvious. I guess it's to make the
>> generated enum a compatible extension of enum QapiSpecialFeature. That
>> one exists for use by schema-independent support code such
>> compat_policy_input_ok() and qobject_output_policy_skip().
>
> Yes, I wanted the overlapping enums vaules to match.
Needs a comment.
>> I further guess you sort the non-special features just to make the
>> generated code easier for humans to navigate.
>>
>> Correct?
>
> The remaining sort was just to give a predictable stable output,
> should QAPI usage of features be reordered.
We don't do that for enum QAPIEvent, and it hasn't inconvenienced us as
far as I can tell. No big deal, I just like consistency.
>> > +
>> > + self._genh.add("typedef enum {\n")
>> > + for f in features:
>> > + self._genh.add(f" {c_enum_const('qapi_feature', f.name)}")
>> > + if f.name in QAPISchemaFeature.SPECIAL_NAMES:
>> > + self._genh.add(f" = {c_enum_const('qapi', f.name)},\n" )
>>
>> pycodestyle gripes
>>
>> scripts/qapi/features.py:51:71: E202 whitespace before ')'
>>
>> > + else:
>> > + self._genh.add(",\n")
>> > +
>> > + self._genh.add("} " + c_name('QapiFeature') + ";\n")
>> > +
>>
>> pycodestyle gripes
>>
>> scripts/qapi/features.py:57:1: E302 expected 2 blank lines, found 1
>>
>> This part generates a C enum. It's similar to gen_enum() from types.py,
>> except we work with a list of QAPISchemaFeature here, and a list of
>> QAPISchemaEnumMember there.
>>
>> To reuse gen_enum() here, we'd have to make up a member list, like we do
>> in events.py for enum QAPIEvent.
>
> I'll have a look at that.
Reuse it only if it's easy for you. We can always improve on top.
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index e97c978d38..5e14b1829b 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):
>> > @@ -1138,6 +1141,11 @@ def __init__(self, fname: str):
>> > self._entity_list: List[QAPISchemaEntity] = []
>> > self._entity_dict: Dict[str, QAPISchemaDefinition] = {}
>> > self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
>> > + self._feature_dict: Dict[str, QAPISchemaFeature] = {}
>> > +
>> > + for f in QAPISchemaFeature.SPECIAL_NAMES:
>> > + self._feature_dict[f] = QAPISchemaFeature(f, "special feature")
>>
>> mypy gripes
>>
>> scripts/qapi/schema.py:1147: error: Argument 2 to "QAPISchemaFeature" has incompatible type "str"; expected "Optional[QAPISourceInfo]" [arg-type]
>>
>> We commonly use None as info value for built-in stuff, and that's why
>> it's Optional[QAPISourceInfo], not just QAPISourceInfo.
>
> Yeah, not sure what I was thinking here, looking again I
> should have passed "None"
>
>> But do we really need to make up some QAPISchemaFeature? Hmm. The
>> appended patch dumbs down ._feature_dict to a set.
See below for a possible reason to keep .feature_dict.
> I was following the same pattern as self._entity_dict and
> self._module_dict, rather than dumbing down to the bare
> minimum needed by my current use case. I don't mind which
> strategy we take.
.entity_dict maps the name to the one entity. Likewise .module_dict.
.feature_dict, however, maps it to the first of possibly many. That's
not wrong, just peculiar and possibly less than obvious to readers who
aren't familiar with how we represent features internally. Worth a
comment?
>> > +
>> > self._schema_dir = os.path.dirname(fname)
>> > self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
>> > self._make_module(fname)
>> > @@ -1258,6 +1266,15 @@ def _make_features(
>> > ) -> List[QAPISchemaFeature]:
>> > if features is None:
>> > return []
>> > +
>> > + for f in features:
>> > + feat = QAPISchemaFeature(f['name'], info)
>> > + if feat.name not in self._feature_dict:
>> > + if len(self._feature_dict) == 64:
>> > + raise Exception("Maximum of 64 schema features is permitted")
>>
>> The limit is an implementation restriction. Okay, we can lift it when
>> it bites us.
>>
>> However, the reporting is less than nice:
>>
>> $ python scripts/qapi-gen.py -o $$ tests/qapi-schema/features-too-many.json
>> Traceback (most recent call last):
>> File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
>> sys.exit(main.main())
>> ^^^^^^^^^^^
>> File "/work/armbru/qemu/scripts/qapi/main.py", line 96, in main
>> generate(args.schema,
>> File "/work/armbru/qemu/scripts/qapi/main.py", line 51, in generate
>> schema = QAPISchema(schema_file)
>> ^^^^^^^^^^^^^^^^^^^^^^^
>> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1155, in __init__
>> self._def_exprs(exprs)
>> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1482, in _def_exprs
>> self._def_struct_type(expr)
>> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1377, in _def_struct_type
>> features = self._make_features(expr.get('features'), info)
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1274, in _make_features
>> raise Exception("Maximum of 64 schema features is permitted")
>> Exception: Maximum of 64 schema features is permitted
>
> Is there any better way to approach this error reporting ?
Raise QAPISemError in .check().
Hmm, then you need a QAPISourceInfo to pass. .feature_dict will give
you one: the .info of the feature added last.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] qapi: expose all schema features to code
2024-11-15 7:47 ` Markus Armbruster
@ 2024-11-15 18:30 ` Daniel P. Berrangé
0 siblings, 0 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 18:30 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Michael Roth, Marc-André Lureau
On Fri, Nov 15, 2024 at 08:47:20AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Thu, Nov 14, 2024 at 01:48:28PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > This replaces use of the constants from the QapiSpecialFeatures
> >> > enum, with constants from the auto-generate QapiFeatures enum
> >> > in qapi-features.h
> >> >
> >> > 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
> >> > retains compatibility with common code that references the features
> >> > via the QapiSpecialFeatures constants.
> >> >
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> > meson.build | 1 +
> >> > scripts/qapi/commands.py | 1 +
> >> > scripts/qapi/features.py | 62 ++++++++++++++++++++++++++++++++++++++++
> >> > scripts/qapi/gen.py | 4 +--
> >> > scripts/qapi/main.py | 2 ++
> >> > scripts/qapi/schema.py | 19 +++++++++++-
> >> > scripts/qapi/types.py | 6 ++--
> >> > scripts/qapi/visit.py | 3 +-
> >> > 8 files changed, 92 insertions(+), 6 deletions(-)
> >> > create mode 100644 scripts/qapi/features.py
> >> I further guess you sort the non-special features just to make the
> >> generated code easier for humans to navigate.
> >>
> >> Correct?
> >
> > The remaining sort was just to give a predictable stable output,
> > should QAPI usage of features be reordered.
>
> We don't do that for enum QAPIEvent, and it hasn't inconvenienced us as
> far as I can tell. No big deal, I just like consistency.
Sure, I'll removethe sorting
> >> pycodestyle gripes
> >>
> >> scripts/qapi/features.py:57:1: E302 expected 2 blank lines, found 1
> >>
> >> This part generates a C enum. It's similar to gen_enum() from types.py,
> >> except we work with a list of QAPISchemaFeature here, and a list of
> >> QAPISchemaEnumMember there.
> >>
> >> To reuse gen_enum() here, we'd have to make up a member list, like we do
> >> in events.py for enum QAPIEvent.
> >
> > I'll have a look at that.
>
> Reuse it only if it's easy for you. We can always improve on top.
Using gen_enum will create the enum <-> string conversion table,
and I'm not sure we need/want that for the special features ?
> >> We commonly use None as info value for built-in stuff, and that's why
> >> it's Optional[QAPISourceInfo], not just QAPISourceInfo.
> >
> > Yeah, not sure what I was thinking here, looking again I
> > should have passed "None"
> >
> >> But do we really need to make up some QAPISchemaFeature? Hmm. The
> >> appended patch dumbs down ._feature_dict to a set.
>
> See below for a possible reason to keep .feature_dict.
>
> > I was following the same pattern as self._entity_dict and
> > self._module_dict, rather than dumbing down to the bare
> > minimum needed by my current use case. I don't mind which
> > strategy we take.
>
> .entity_dict maps the name to the one entity. Likewise .module_dict.
> .feature_dict, however, maps it to the first of possibly many. That's
> not wrong, just peculiar and possibly less than obvious to readers who
> aren't familiar with how we represent features internally. Worth a
> comment?
I'll comment it.
> >> However, the reporting is less than nice:
> >>
> >> $ python scripts/qapi-gen.py -o $$ tests/qapi-schema/features-too-many.json
> >> Traceback (most recent call last):
> >> File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
> >> sys.exit(main.main())
> >> ^^^^^^^^^^^
> >> File "/work/armbru/qemu/scripts/qapi/main.py", line 96, in main
> >> generate(args.schema,
> >> File "/work/armbru/qemu/scripts/qapi/main.py", line 51, in generate
> >> schema = QAPISchema(schema_file)
> >> ^^^^^^^^^^^^^^^^^^^^^^^
> >> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1155, in __init__
> >> self._def_exprs(exprs)
> >> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1482, in _def_exprs
> >> self._def_struct_type(expr)
> >> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1377, in _def_struct_type
> >> features = self._make_features(expr.get('features'), info)
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> File "/work/armbru/qemu/scripts/qapi/schema.py", line 1274, in _make_features
> >> raise Exception("Maximum of 64 schema features is permitted")
> >> Exception: Maximum of 64 schema features is permitted
> >
> > Is there any better way to approach this error reporting ?
>
> Raise QAPISemError in .check().
>
> Hmm, then you need a QAPISourceInfo to pass. .feature_dict will give
> you one: the .info of the feature added last.
This also points out that I failed to add a test case for this
"too many features' scenario, which I'll fix too.
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 :|
^ permalink raw reply [flat|nested] 12+ messages in thread