Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: jsnow@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com
Subject: Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
Date: Wed, 27 Jan 2021 14:06:53 +0100	[thread overview]
Message-ID: <87tur2plzm.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201112172850.401925-3-kwolf@redhat.com> (Kevin Wolf's message of "Thu, 12 Nov 2020 18:28:46 +0100")

Kevin Wolf <kwolf@redhat.com> writes:

> This makes qobject-input-visitor remember the currently valid aliases in
> each StackObject. It doesn't actually allow using the aliases yet.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 23843b242e..a00ac32682 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -29,6 +29,29 @@
>  #include "qemu/cutils.h"
>  #include "qemu/option.h"
>  
> +typedef struct InputVisitorAlias {
> +   /* Alias name. NULL for any (wildcard alias). */
> +    const char *alias;
> +
> +    /*
> +     * NULL terminated array representing a path.
> +     * Must contain at least one non-NULL element if alias is not NULL.

What does .alias = NULL, .src[] empty mean?

The previous patch's contract for visit_define_alias():

   /*
    * 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
    * 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.
    *
    * @source is a NULL-terminated array of names that describe the path to
    * a member, starting from the currently visited struct.
    *
    * 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.
    */

If I read this correctly, then empty .src[] denotes "the current
struct", and therefore .alias = NULL, .src[] empty means "all members of
the current struct are considered to have alias members with the same
key in the current struct".  Which is be a complicated way to accomplish
nothing.

Am I confused?

> +     */
> +    const char **src;
> +
> +    /* StackObject in which the alias should be looked for */
> +    struct StackObject *alias_so;

Clear as mud.  Is it "the current struct"?  If not, what else?  Perhaps
an example would help me understand.

> +
> +    /*
> +     * The alias remains valid as long as the containing StackObject has

What's "the containing StackObject"?  I guess it's the one that has this
InputVisitorAlias in .aliases.

> +     * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
> +     * or until the whole StackObject is removed.
> +     */
> +    int scope_nesting;
> +
> +    QSLIST_ENTRY(InputVisitorAlias) next;
> +} InputVisitorAlias;
> +
>  typedef struct StackObject {
>      const char *name;            /* Name of @obj in its parent, if any */
>      QObject *obj;                /* QDict or QList being visited */
> @@ -38,6 +61,9 @@ typedef struct StackObject {
>      const QListEntry *entry;    /* If @obj is QList: unvisited tail */
>      unsigned index;             /* If @obj is QList: list index of @entry */
>  
> +    QSLIST_HEAD(, InputVisitorAlias) aliases;
> +    int alias_scope_nesting;    /* Increase on scope start, decrease on end */

I get what the comment means, but imperative mood is odd for a variable.
"Number of open scopes", perhaps?

> +
>      QSLIST_ENTRY(StackObject) node; /* parent */
>  } StackObject;
>  

I'm afraid I'm too confused about the interface (previous patch) and the
data structures to continue review with reasonable efficiency.  I don't
mean to imply either is bad!  I'm just confused, that's all.

[...]