Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: jsnow@redhat.com, qemu-devel@nongnu.org
Subject: Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
Date: Wed, 27 Jan 2021 13:51:33 +0100	[thread overview]
Message-ID: <87czxqr19m.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201112172850.401925-2-kwolf@redhat.com> (Kevin Wolf's message of "Thu, 12 Nov 2020 18:28:45 +0100")

Kevin Wolf <kwolf@redhat.com> writes:

> This adds functions to the Visitor interface that can be used to define
> aliases and alias scopes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/visitor-impl.h | 12 ++++++++++++
>  include/qapi/visitor.h      | 37 +++++++++++++++++++++++++++++++++++++
>  qapi/qapi-visit-core.c      | 21 +++++++++++++++++++++
>  3 files changed, 70 insertions(+)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 7362c043be..e30da2599c 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -113,6 +113,18 @@ struct Visitor
>         The core takes care of the return type in the public interface. */
>      void (*optional)(Visitor *v, const char *name, bool *present);
>  
> +    /*
> +     * Optional; intended for input visitors. If not given, aliases are
> +     * ignored.
> +     */
> +    void (*define_alias)(Visitor *v, const char *alias, const char **source);
> +
> +    /* Must be set if define_alias is set */
> +    void (*start_alias_scope)(Visitor *v);
> +
> +    /* Must be set if define_alias is set */
> +    void (*end_alias_scope)(Visitor *v);
> +
>      /* Must be set */
>      VisitorType type;
>  
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index ebc19ede7f..9bdc0ee03d 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
>   */
>  bool visit_optional(Visitor *v, const char *name, bool *present);
>  
> +/*
> + * Defines a new alias rule.
> + *
> + * If @alias is non-NULL, the member named @alias in the external
> + * representation of the current struct is defined as an alias for the

Terminology: the big comment uses "object".  See also the FIXME in
visit_start_struct()'s contract.

> + * member described by @source.
> + *
> + * If @alias is NULL, all members of the struct described by @source are
> + * considered to have alias members with the same key in the current
> + * struct.

Define "the current struct".  I believe it's the object being visited.

What happens if we're currently visiting something other than an object,
i.e. the root of a tree, or a list?

> + *
> + * @source is a NULL-terminated array of names that describe the path to
> + * a member, starting from the currently visited struct.

I'm afraid "describe the path to a member" is too vague.  How?

I figure this is what you have in mind:

    cur = the currently visited object
    for s in source:
        cur = the member of cur denoted by s

When @cur is indeed an object, then "the member denoted by @s" makes
sense: you must pass a name when visiting object members, and whatever
is visited with name @s is the member denoted by @s.

"Must pass a name" is documented in the big comment:

 * The @name parameter of visit_type_FOO() describes the relation
 * between this QAPI value and its parent container.  When visiting
 * the root of a tree, @name is ignored; when visiting a member of an
 * object, @name is the key associated with the value; when visiting a
 * member of a list, @name is NULL; and when visiting the member of an
 * alternate, @name should equal the name used for visiting the
 * alternate.

But what if @cur is a list?  I guess that makes no sense.  Say so
explicitly, please.

> + *
> + * The alias stays valid until the current alias scope ends.
> + * visit_start/end_struct() implicitly start/end an alias scope.
> + * Additionally, visit_start/end_alias_scope() can be used to explicitly
> + * create a nested alias scope.
> + */
> +void visit_define_alias(Visitor *v, const char *alias, const char **source);
> +
> +/*
> + * Begins an explicit alias scope.
> + *
> + * Alias definitions after here will only stay valid until the
> + * corresponding visit_end_alias_scope() is called.
> + */
> +void visit_start_alias_scope(Visitor *v);
> +
> +/*
> + * Ends an explicit alias scope.
> + *
> + * Alias definitions between the correspoding visit_start_alias_scope()
> + * call and here go out of scope and won't apply in later code any more.
> + */
> +void visit_end_alias_scope(Visitor *v);
> +
>  /*
>   * Visit an enum value.
>   *
[...]