[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking |
Date: |
Fri, 29 Oct 2021 18:13:29 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
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.
>
- [PATCH v2 7/9] qapi: Generalize enum member policy checking, (continued)
[PATCH v2 1/9] qapi: New special feature flag "unstable", Markus Armbruster, 2021/10/28
[PATCH v2 2/9] qapi: Mark unstable QMP parts with feature 'unstable', Markus Armbruster, 2021/10/28
[PATCH v2 6/9] qapi: Generalize command policy checking, Markus Armbruster, 2021/10/28
[PATCH v2 9/9] qapi: Extend -compat to set policy for unstable interfaces, Markus Armbruster, 2021/10/28
[PATCH v2 8/9] qapi: Factor out compat_policy_input_ok(), Markus Armbruster, 2021/10/28