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 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

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>

Reviewed-by: Juan Quintela <quintela@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.

Juan Quintela <quintela@redhat.com> writes:

> 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>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Reversing accept/reject make things "interesting" for a review point of view.

Sorry about that.

>> + * @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?

Not to me, I'm afraid...

Thanks!

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@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@redhat.com>

>      default:
>          abort();

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> 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@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();

Maybe, but making it so has nothing to do with this patch.  It could
perhaps be done in PATCH 8, or in a followup patch.

> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Okay to tack your R-by to the unmodified patch?

Thanks!

>
>>      default:
>>          abort();

On 10/29/21 16:01, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> 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@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(-)

>>>      case COMPAT_POLICY_INPUT_CRASH:
>>
>> Clearer as:
>>
>>            abort();
>>        default:
>>            g_assert_not_reached();
> 
> Maybe, but making it so has nothing to do with this patch.  It could
> perhaps be done in PATCH 8, or in a followup patch.
> 
>> Otherwise,
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Okay to tack your R-by to the unmodified patch?

Sure.

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@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

Eric Blake <eblake@redhat.com> writes:

> 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@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?

Good catch!

These functions are called indirectly like

    if (visit_policy_reject(v, "values", 1u << QAPI_DEPRECATED, errp)) {
        return false;
    }
    if (!visit_policy_skip(v, "values", 1u << QAPI_DEPRECATED)) {
        if (!visit_type_strList(v, "values", &obj->values, errp)) {
            return false;
        }
    }

visit_policy_reject() must return true when it sets an error, or else we
call visit_policy_skip() with @errp pointing to non-null.

Same argument for visit_policy_skip().

>>      }
>> -    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.

On 10/29/21 17:34, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> 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@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?
> 
> Good catch!

Ouch. I admit this patch logic is hard to review, but couldn't come
with a nice way to split it further.

> These functions are called indirectly like
> 
>     if (visit_policy_reject(v, "values", 1u << QAPI_DEPRECATED, errp)) {
>         return false;
>     }
>     if (!visit_policy_skip(v, "values", 1u << QAPI_DEPRECATED)) {
>         if (!visit_type_strList(v, "values", &obj->values, errp)) {
>             return false;
>         }
>     }
> 
> visit_policy_reject() must return true when it sets an error, or else we
> call visit_policy_skip() with @errp pointing to non-null.
> 
> Same argument for visit_policy_skip().
> 
>>>      }
>>> -    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.
>