On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <arm...@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 <arm...@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 49136ae88e..b4a81f1757 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..fda485614b 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..b5c6564cbb 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': > Would it be possible for gen_special_features to return something false-y instead of '0'? Do we actually *use* the '0' return anywhere other than to test it to see if we should include additional code? If you actually use the '0' anywhere: Go ahead and treat this as an ack. If you don't, can we clean this up? (Sorry, I find the mcgen stuff hard to read in patch form and I am trying to give you a quick review instead of NO review.) --js > 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 > >