Libguestfs - Oct 2021 - [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 <armbru at 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?
>      }
> -    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

Eric Blake <eblake at 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 at 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!

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.