Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking

[Markus Armbruster]

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