On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@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@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