Markus Armbruster
2021-Oct-28 10:25 UTC
[Libguestfs] [PATCH v2 5/9] qapi: Generalize struct member policy checking
The generated visitor functions call visit_deprecated_accept() and
visit_deprecated() when visiting a struct member with special feature
flag 'deprecated'. This makes the feature flag visible to the actual
visitors. I want to make feature flag 'unstable' visible there as
well, so I can add policy for it.
To let me make it visible, replace these functions by
visit_policy_reject() and visit_policy_skip(), which take the member's
special features as an argument. Note that the new functions have the
opposite sense, i.e. the return value flips.
Signed-off-by: Markus Armbruster <armbru at redhat.com>
---
include/qapi/visitor-impl.h | 6 ++++--
include/qapi/visitor.h | 17 +++++++++++++----
qapi/qapi-forward-visitor.c | 16 +++++++++-------
qapi/qapi-visit-core.c | 22 ++++++++++++----------
qapi/qobject-input-visitor.c | 15 ++++++++++-----
qapi/qobject-output-visitor.c | 9 ++++++---
qapi/trace-events | 4 ++--
scripts/qapi/visit.py | 14 +++++++-------
8 files changed, 63 insertions(+), 40 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 72b6537bef..2badec5ba4 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -114,10 +114,12 @@ struct Visitor
void (*optional)(Visitor *v, const char *name, bool *present);
/* Optional */
- bool (*deprecated_accept)(Visitor *v, const char *name, Error **errp);
+ bool (*policy_reject)(Visitor *v, const char *name,
+ unsigned special_features, Error **errp);
/* Optional */
- bool (*deprecated)(Visitor *v, const char *name);
+ bool (*policy_skip)(Visitor *v, const char *name,
+ unsigned special_features);
/* Must be set */
VisitorType type;
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index dcb96018a9..d53a84c9ba 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -461,22 +461,31 @@ void visit_end_alternate(Visitor *v, void **obj);
bool visit_optional(Visitor *v, const char *name, bool *present);
/*
- * Should we reject deprecated member @name?
+ * Should we reject member @name due to policy?
+ *
+ * @special_features is the member's special features encoded as a
+ * bitset of QapiSpecialFeature.
*
* @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_deprecated_accept(Visitor *v, const char *name, Error **errp);
+bool visit_policy_reject(Visitor *v, const char *name,
+ unsigned special_features, Error **errp);
/*
- * Should we visit deprecated member @name?
+ *
+ * Should we skip member @name due to policy?
+ *
+ * @special_features is the member's special features encoded as a
+ * bitset of QapiSpecialFeature.
*
* @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_deprecated(Visitor *v, const char *name);
+bool visit_policy_skip(Visitor *v, const char *name,
+ unsigned special_features);
/*
* Set policy for handling deprecated management interfaces.
diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
index a4b111e22a..25d098aa8a 100644
--- a/qapi/qapi-forward-visitor.c
+++ b/qapi/qapi-forward-visitor.c
@@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const char
*name, bool *present)
visit_optional(ffv->target, name, present);
}
-static bool forward_field_deprecated_accept(Visitor *v, const char *name,
- Error **errp)
+static bool forward_field_policy_reject(Visitor *v, const char *name,
+ unsigned special_features,
+ Error **errp)
{
ForwardFieldVisitor *ffv = to_ffv(v);
if (!forward_field_translate_name(ffv, &name, errp)) {
return false;
}
- return visit_deprecated_accept(ffv->target, name, errp);
+ return visit_policy_reject(ffv->target, name, special_features, errp);
}
-static bool forward_field_deprecated(Visitor *v, const char *name)
+static bool forward_field_policy_skip(Visitor *v, const char *name,
+ unsigned special_features)
{
ForwardFieldVisitor *ffv = to_ffv(v);
if (!forward_field_translate_name(ffv, &name, NULL)) {
return false;
}
- return visit_deprecated(ffv->target, name);
+ return visit_policy_skip(ffv->target, name, special_features);
}
static void forward_field_complete(Visitor *v, void *opaque)
@@ -313,8 +315,8 @@ Visitor *visitor_forward_field(Visitor *target, const char
*from, const char *to
v->visitor.type_any = forward_field_type_any;
v->visitor.type_null = forward_field_type_null;
v->visitor.optional = forward_field_optional;
- v->visitor.deprecated_accept = forward_field_deprecated_accept;
- v->visitor.deprecated = forward_field_deprecated;
+ v->visitor.policy_reject = forward_field_policy_reject;
+ v->visitor.policy_skip = forward_field_policy_skip;
v->visitor.complete = forward_field_complete;
v->visitor.free = forward_field_free;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 617ef3fa46..f95503cbec 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -139,22 +139,24 @@ bool visit_optional(Visitor *v, const char *name, bool
*present)
return *present;
}
-bool visit_deprecated_accept(Visitor *v, const char *name, Error **errp)
+bool visit_policy_reject(Visitor *v, const char *name,
+ unsigned special_features, Error **errp)
{
- trace_visit_deprecated_accept(v, name);
- if (v->deprecated_accept) {
- return v->deprecated_accept(v, name, errp);
+ trace_visit_policy_reject(v, name);
+ if (v->policy_reject) {
+ return v->policy_reject(v, name, special_features, errp);
}
- return true;
+ return false;
}
-bool visit_deprecated(Visitor *v, const char *name)
+bool visit_policy_skip(Visitor *v, const char *name,
+ unsigned special_features)
{
- trace_visit_deprecated(v, name);
- if (v->deprecated) {
- return v->deprecated(v, name);
+ trace_visit_policy_skip(v, name);
+ if (v->policy_skip) {
+ return v->policy_skip(v, name, special_features);
}
- return true;
+ return false;
}
void visit_set_policy(Visitor *v, CompatPolicy *policy)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 71b24a4429..c120dbdd92 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -662,16 +662,21 @@ static void qobject_input_optional(Visitor *v, const char
*name, bool *present)
*present = true;
}
-static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
- Error **errp)
+static bool qobject_input_policy_reject(Visitor *v, const char *name,
+ unsigned special_features,
+ Error **errp)
{
+ if (!(special_features & 1u << QAPI_DEPRECATED)) {
+ return false;
+ }
+
switch (v->compat_policy.deprecated_input) {
case COMPAT_POLICY_INPUT_ACCEPT:
- return true;
+ return false;
case COMPAT_POLICY_INPUT_REJECT:
error_setg(errp, "Deprecated parameter '%s' disabled by
policy",
name);
- return false;
+ return true;
case COMPAT_POLICY_INPUT_CRASH:
default:
abort();
@@ -712,7 +717,7 @@ static QObjectInputVisitor
*qobject_input_visitor_base_new(QObject *obj)
v->visitor.end_list = qobject_input_end_list;
v->visitor.start_alternate = qobject_input_start_alternate;
v->visitor.optional = qobject_input_optional;
- v->visitor.deprecated_accept = qobject_input_deprecated_accept;
+ v->visitor.policy_reject = qobject_input_policy_reject;
v->visitor.free = qobject_input_free;
v->root = qobject_ref(obj);
diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
index 9b7f510036..b155bf4149 100644
--- a/qapi/qobject-output-visitor.c
+++ b/qapi/qobject-output-visitor.c
@@ -13,6 +13,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/compat-policy.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/visitor-impl.h"
#include "qemu/queue.h"
@@ -208,9 +209,11 @@ static bool qobject_output_type_null(Visitor *v, const char
*name,
return true;
}
-static bool qobject_output_deprecated(Visitor *v, const char *name)
+static bool qobject_output_policy_skip(Visitor *v, const char *name,
+ unsigned special_features)
{
- return v->compat_policy.deprecated_output != COMPAT_POLICY_OUTPUT_HIDE;
+ return !(special_features & 1u << QAPI_DEPRECATED)
+ || v->compat_policy.deprecated_output == COMPAT_POLICY_OUTPUT_HIDE;
}
/* Finish building, and return the root object.
@@ -262,7 +265,7 @@ Visitor *qobject_output_visitor_new(QObject **result)
v->visitor.type_number = qobject_output_type_number;
v->visitor.type_any = qobject_output_type_any;
v->visitor.type_null = qobject_output_type_null;
- v->visitor.deprecated = qobject_output_deprecated;
+ v->visitor.policy_skip = qobject_output_policy_skip;
v->visitor.complete = qobject_output_complete;
v->visitor.free = qobject_output_free;
diff --git a/qapi/trace-events b/qapi/trace-events
index cccafc07e5..ab108c4f0e 100644
--- a/qapi/trace-events
+++ b/qapi/trace-events
@@ -17,8 +17,8 @@ visit_start_alternate(void *v, const char *name, void *obj,
size_t size) "v=%p n
visit_end_alternate(void *v, void *obj) "v=%p obj=%p"
visit_optional(void *v, const char *name, bool *present) "v=%p name=%s
present=%p"
-visit_deprecated_accept(void *v, const char *name) "v=%p name=%s"
-visit_deprecated(void *v, const char *name) "v=%p name=%s"
+visit_policy_reject(void *v, const char *name) "v=%p name=%s"
+visit_policy_skip(void *v, const char *name) "v=%p name=%s"
visit_type_enum(void *v, const char *name, int *obj) "v=%p name=%s
obj=%p"
visit_type_int(void *v, const char *name, int64_t *obj) "v=%p name=%s
obj=%p"
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 9d9196a143..e13bbe4292 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -21,7 +21,7 @@
indent,
mcgen,
)
-from .gen import QAPISchemaModularCVisitor, ifcontext
+from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext
from .schema import (
QAPISchema,
QAPISchemaEnumMember,
@@ -76,7 +76,6 @@ def gen_visit_object_members(name: str,
c_type=base.c_name())
for memb in members:
- deprecated = 'deprecated' in [f.name for f in memb.features]
ret += memb.ifcond.gen_if()
if memb.optional:
ret += mcgen('''
@@ -84,14 +83,15 @@ def gen_visit_object_members(name: str,
''',
name=memb.name, c_name=c_name(memb.name))
indent.increase()
- if deprecated:
+ special_features = gen_special_features(memb.features)
+ if special_features != '0':
ret += mcgen('''
- if (!visit_deprecated_accept(v, "%(name)s", errp)) {
+ if (visit_policy_reject(v, "%(name)s", %(special_features)s,
errp)) {
return false;
}
- if (visit_deprecated(v, "%(name)s")) {
+ if (!visit_policy_skip(v, "%(name)s", %(special_features)s)) {
''',
- name=memb.name)
+ name=memb.name, special_features=special_features)
indent.increase()
ret += mcgen('''
if (!visit_type_%(c_type)s(v, "%(name)s",
&obj->%(c_name)s, errp)) {
@@ -100,7 +100,7 @@ def gen_visit_object_members(name: str,
''',
c_type=memb.type.c_name(), name=memb.name,
c_name=c_name(memb.name))
- if deprecated:
+ if special_features != '0':
indent.decrease()
ret += mcgen('''
}
--
2.31.1
Juan Quintela
2021-Oct-29 10:42 UTC
[Libguestfs] [PATCH v2 5/9] qapi: Generalize struct member policy checking
Markus Armbruster <armbru at redhat.com> wrote:> The generated visitor functions call visit_deprecated_accept() and > visit_deprecated() when visiting a struct member with special feature > flag 'deprecated'. This makes the feature flag visible to the actual > visitors. I want to make feature flag 'unstable' visible there as > well, so I can add policy for it. > > To let me make it visible, replace these functions by > visit_policy_reject() and visit_policy_skip(), which take the member's > special features as an argument. Note that the new functions have the > opposite sense, i.e. the return value flips. > > Signed-off-by: Markus Armbruster <armbru at redhat.com>Reviewed-by: Juan Quintela <quintela at redhat.com> Reversing accept/reject make things "interesting" for a review point of view.> + * @special_features is the member's special features encoded as a > + * bitset of QapiSpecialFeature.Just to nitty pick, if you rename the variable to features, does the sentece is clearer? Later, Juan.
Philippe Mathieu-Daudé
2021-Oct-29 13:31 UTC
[Libguestfs] [PATCH v2 5/9] qapi: Generalize struct member policy checking
On 10/28/21 12:25, Markus Armbruster wrote:> The generated visitor functions call visit_deprecated_accept() and > visit_deprecated() when visiting a struct member with special feature > flag 'deprecated'. This makes the feature flag visible to the actual > visitors. I want to make feature flag 'unstable' visible there as > well, so I can add policy for it. > > To let me make it visible, replace these functions by > visit_policy_reject() and visit_policy_skip(), which take the member's > special features as an argument. Note that the new functions have the > opposite sense, i.e. the return value flips. > > Signed-off-by: Markus Armbruster <armbru at redhat.com> > --- > include/qapi/visitor-impl.h | 6 ++++-- > include/qapi/visitor.h | 17 +++++++++++++---- > qapi/qapi-forward-visitor.c | 16 +++++++++------- > qapi/qapi-visit-core.c | 22 ++++++++++++---------- > qapi/qobject-input-visitor.c | 15 ++++++++++----- > qapi/qobject-output-visitor.c | 9 ++++++--- > qapi/trace-events | 4 ++-- > scripts/qapi/visit.py | 14 +++++++------- > 8 files changed, 63 insertions(+), 40 deletions(-)> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name, > - Error **errp) > +static bool qobject_input_policy_reject(Visitor *v, const char *name, > + unsigned special_features, > + Error **errp) > { > + if (!(special_features & 1u << QAPI_DEPRECATED)) { > + return false; > + } > + > switch (v->compat_policy.deprecated_input) { > case COMPAT_POLICY_INPUT_ACCEPT: > - return true; > + return false; > case COMPAT_POLICY_INPUT_REJECT: > error_setg(errp, "Deprecated parameter '%s' disabled by policy", > name); > - return false; > + return true; > case COMPAT_POLICY_INPUT_CRASH:Clearer as: abort(); default: g_assert_not_reached(); Otherwise, Reviewed-by: Philippe Mathieu-Daud? <philmd at redhat.com>> default: > abort();
Eric Blake
2021-Oct-29 15:20 UTC
[Libguestfs] [PATCH v2 5/9] qapi: Generalize struct member policy checking
On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:> The generated visitor functions call visit_deprecated_accept() and > visit_deprecated() when visiting a struct member with special feature > flag 'deprecated'. This makes the feature flag visible to the actual > visitors. I want to make feature flag 'unstable' visible there as > well, so I can add policy for it. > > To let me make it visible, replace these functions by > visit_policy_reject() and visit_policy_skip(), which take the member's > special features as an argument. Note that the new functions have the > opposite sense, i.e. the return value flips. > > Signed-off-by: Markus Armbruster <armbru at redhat.com> > ---> +++ b/qapi/qapi-forward-visitor.c > @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const char *name, bool *present) > visit_optional(ffv->target, name, present); > } > > -static bool forward_field_deprecated_accept(Visitor *v, const char *name, > - Error **errp) > +static bool forward_field_policy_reject(Visitor *v, const char *name, > + unsigned special_features, > + Error **errp) > { > ForwardFieldVisitor *ffv = to_ffv(v); > > if (!forward_field_translate_name(ffv, &name, errp)) { > return false;Should this return value be flipped?> } > - return visit_deprecated_accept(ffv->target, name, errp); > + return visit_policy_reject(ffv->target, name, special_features, errp); > } > > -static bool forward_field_deprecated(Visitor *v, const char *name) > +static bool forward_field_policy_skip(Visitor *v, const char *name, > + unsigned special_features) > { > ForwardFieldVisitor *ffv = to_ffv(v); > > if (!forward_field_translate_name(ffv, &name, NULL)) { > return false;and here too?> } > - return visit_deprecated(ffv->target, name); > + return visit_policy_skip(ffv->target, name, special_features); > } >Otherwise, the rest of the logic changes for flipped sense look right. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org