Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On 10/29/21 17:34, Markus Armbruster wrote:
> Eric Blake 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
>>> ---
>>
>>> +++ 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.
>
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On 10/29/21 16:01, Markus Armbruster wrote: > Philippe Mathieu-Daudé 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 >>> --- >>> 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é > > Okay to tack your R-by to the unmodified patch? Sure.
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Eric Blake 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
>> ---
>
>> +++ 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.
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
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
> ---
> +++ 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
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Philippe Mathieu-Daudé 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
>> ---
>> 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é
Okay to tack your R-by to the unmodified patch?
Thanks!
>
>> default:
>> abort();
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
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
> ---
> 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é
> default:
> abort();
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Juan Quintela writes: > 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 > > Reviewed-by: Juan Quintela > > 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!
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
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 Reviewed-by: Juan Quintela 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.
[PATCH v2 5/9] qapi: Generalize struct member policy checking
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)
@@ -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.de
