[PATCH 5/9] qapi: Generalize struct member policy checking
John Snow
jsnow at redhat.com
Mon Oct 25 19:28:15 UTC 2021
On Mon, Oct 25, 2021 at 1:25 AM 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>
> ---
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211025/ba08b823/attachment-0001.htm>
More information about the libvir-list
mailing list