Re: [PATCH 5/9] qapi: Generalize struct member policy checking
Philippe Mathieu-Daudé writes:
> On 10/25/21 07: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
>> ---
>> 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/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)) {
>
> Unreachable =) Proof than extract() is safer :P
Good eyes, thank you!
I actually like extract & desposit macros when the width is greater than
one. Then, the longhand C code is illegible anyway, and having to
remember what the macros mean is no worse.
For width 1 it feels like a wash. Universal use of the macros could
build familiarity and thus tip the balance.
I count more than a thousand instances of '& (1 <<'.
I wasn't even aware the macros existed in QEMU[*].
>
>> +return false;
>> +}
[*] I may well have seen them before, but my memory is limited and
lossy.
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
On 10/25/21 07: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
> ---
> 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/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)) {
Unreachable =) Proof than extract() is safer :P
> +return false;
> +}
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
John Snow writes:
> On Mon, Oct 25, 2021 at 1:25 AM 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
[...]
>> 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
>>
>>
Re: [PATCH 5/9] qapi: Generalize struct member policy checking
On Mon, Oct 25, 2021 at 1:25 AM 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
> ---
> 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)
>
