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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

[...]

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

gen_special_features() returns a string holding C code for a special
features bitset.  The empty bitset is 0.

The "natural" use is interpolating this value into C code.  Two
instances are visible below.

The use right here is for testing "have we got special features", so we
can skip generating code that is of no use when we don't have any.  It's
admittedly slightly unclean.

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

Any kind of review is appreciated :)

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

These are the "natural" uses I mentioned.

If gen_special_features() returned something other than '0', we'd have
to replace it by '0' here.

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